Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions docs/reference/experiments.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ The following is a list of currently available experiments. We'll remove experim

* `bolt-install`: enables creating, installing, and running Bolt projects that manage their app manifest on app settings (remote manifest).
* `slack create` and `slack init` now set manifest source to "app settings" (remote) for Bolt JS & Bolt Python projects ([PR#96](https://github.com/slackapi/slack-cli/pull/96)).
* `slack run` supports creating and installing Bolt Framework apps that have the manifest source set to "app settings (remote)" ([PR#111](https://github.com/slackapi/slack-cli/pull/111)).
* `read-only-collaborators`: enables creating and modifying collaborator permissions via the `slack collaborator` commands.

## Experiments changelog
Expand Down
51 changes: 43 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,42 @@ 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. After an app is created,
// app settings becomes the source of truth for remote manifests, so updates and installs always
// get the latest manifest from app settings.
// When the BoltInstall experiment is disabled, we get the manifest from the local file because
// this is how the original implementation worked.
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 +496,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 +512,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 +673,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