Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions internal/pkg/apps/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,11 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
if err != nil {
return types.App{}, api.DeveloperAppInstallResult{}, "", err
}
if !manifestUpdates && !manifestCreates {
return app, api.DeveloperAppInstallResult{}, "", nil

if !clients.Config.WithExperimentOn(experiment.BoltInstall) {
if !manifestUpdates && !manifestCreates {
return app, api.DeveloperAppInstallResult{}, "", nil
}
Comment on lines +344 to +347
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: InstallLocalApp now continues even with there are no manifest updates or creates. This is because it must still install the app in order to get the tokens.

}

apiInterface := clients.API()
Expand All @@ -365,17 +368,39 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
clients.EventTracker.SetAuthEnterpriseID(*authSession.EnterpriseID)
}

slackYaml, err := clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor)
if err != nil {
return app, api.DeveloperAppInstallResult{}, "", err
// When the BoltInstall experiment is enabled, we need to get the manifest from the local file
// if the manifest source is local or if we are creating a new app. Otherwise, we get the manifest
// from the app settings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwbrooks Small suggestion - it might be nice to note why we're collecting a remote manifest here? 📚

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimeg Thanks for catching this! Commit 1b3238c clarifies why we get the manifest from the local file when creating (or when the manifest source is local), while using app settings for updates and installations. 📝

var slackManifest types.SlackYaml
if clients.Config.WithExperimentOn(experiment.BoltInstall) {
manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)
if err != nil {
return app, api.DeveloperAppInstallResult{}, "", err
}
if manifestSource.Equals(config.ManifestSourceLocal) || manifestCreates {
slackManifest, err = clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor)
if err != nil {
return app, api.DeveloperAppInstallResult{}, "", err
}
} else {
slackManifest, err = clients.AppClient().Manifest.GetManifestRemote(ctx, auth.Token, app.AppID)
if err != nil {
return app, api.DeveloperAppInstallResult{}, "", err
}
}
} else {
slackManifest, err = clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor)
if err != nil {
return app, api.DeveloperAppInstallResult{}, "", err
}
}

log.Data["appName"] = slackYaml.DisplayInformation.Name
log.Data["appName"] = slackManifest.DisplayInformation.Name
log.Data["isUpdate"] = app.AppID != ""
log.Data["teamName"] = *authSession.TeamName
log.Log("INFO", "app_install_manifest")

manifest := slackYaml.AppManifest
manifest := slackManifest.AppManifest
appendLocalToDisplayName(&manifest)
if manifest.IsFunctionRuntimeSlackHosted() {
configureLocalManifest(ctx, clients, &manifest)
Expand Down Expand Up @@ -468,6 +493,7 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
log.Info("app_install_start")
var installState types.InstallState
result, installState, err := apiInterface.DeveloperAppInstall(ctx, clients.IO, token, app, botScopes, outgoingDomains, orgGrantWorkspaceID, clients.Config.AutoRequestAAAFlag)

if err != nil {
err = slackerror.Wrap(err, slackerror.ErrAppInstall)
return app, api.DeveloperAppInstallResult{}, "", err
Expand All @@ -483,7 +509,7 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
}

//
// TODO(@mbrevoort) - Currently, cannot update the icon if app is not hosted
// TODO: Currently, cannot update the icon if app is not hosted.
//
// upload icon, default to icon.png
// var iconPath = slackYaml.Icon
Expand Down Expand Up @@ -644,6 +670,12 @@ func shouldCreateManifest(ctx context.Context, clients *shared.ClientFactory, ap
if !clients.Config.WithExperimentOn(experiment.BoltFrameworks) {
return app.AppID == "", nil
}

// When the BoltInstall experiment is enabled, apps can always be created with any manifest source.
if clients.Config.WithExperimentOn(experiment.BoltInstall) {
return app.AppID == "", nil
}
Comment on lines +677 to +680
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: shouldCreateManifest now always returns true when the experiment is turned on because both local and remote manifest sources can be created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwbrooks This is so good! 🧠 ✨

As later follow up we might want to wrap errors from .GetManifestLocal to suggest linking an created on app on app settings if the get-manifest hook errors for some reason and the manifest source is remote.

No blocker for this PR since IMO error handling is another discussion altogether!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: This might be best to note for the conclusion of this experiment to let us streamline some of the logic above and make this case more clear 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really great suggestion! I think it would help developers move forward who don't have a local manifest file. I've made a note to follow-up on this error suggestion. ✏️


manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)
if err != nil {
return false, err
Expand Down
Loading
Loading