-
Notifications
You must be signed in to change notification settings - Fork 24
feat(bolt-install): support using 'install' command with remote manifests #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 63.03% 63.05% +0.02%
==========================================
Files 212 212
Lines 21608 21667 +59
==========================================
+ Hits 13621 13663 +42
- Misses 6942 6952 +10
- Partials 1045 1052 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧘🏻 Comments for the kind reviewers!
| manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if manifestSource.Equals(config.ManifestSourceRemote) { | ||
| return slackerror.New(slackerror.ErrAppInstall). | ||
| WithMessage("Apps cannot be installed due to project configurations"). | ||
| WithRemediation( | ||
| "Install an app on app settings: %s\nLink an app to this project with %s\nList apps saved with this project using %s", | ||
| style.LinkText("https://api.slack.com/apps"), | ||
| style.Commandf("app link", false), | ||
| style.Commandf("app list", false), | ||
| ). | ||
| WithDetails(slackerror.ErrorDetails{ | ||
| slackerror.ErrorDetail{ | ||
| Code: slackerror.ErrProjectConfigManifestSource, | ||
| Message: "Cannot install apps with manifests sourced from app settings", | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 Removing this section that prevents the slack install command from executing on a remote manifest.
cmd/app/add.go
Outdated
| // Prompt for deployed or local app environment. | ||
| isProductionApp, err := promptIsProduction(ctx, clients) | ||
| if err != nil { | ||
| return ctx, "", types.App{}, err | ||
| } | ||
|
|
||
| var appEnvironmentType prompts.AppEnvironmentType | ||
| if isProductionApp { | ||
| appEnvironmentType = prompts.ShowHostedOnly | ||
| } else { | ||
| appEnvironmentType = prompts.ShowLocalOnly | ||
| } | ||
|
|
||
| selected, err := teamAppSelectPromptFunc(ctx, clients, appEnvironmentType, prompts.ShowAllApps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: New code that prompts the developer to choose an environment (local or deployed). Then displays the proper Team Prompt. We cannot prompts.ShowAllEnvironments because there is guard logic to avoid having 2 apps on one team. Unfortunately, the prompt isn't aware of the app's environment, so this was our workaround.
cmd/app/add.go
Outdated
| if !isProductionApp { | ||
| selection.App.IsDev = true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Normally, we'd also update the UserID for dev apps, but it's already been set.
| "errors if manifest.source is remote with the bolt experiment": { | ||
| ExpectedError: slackerror.New(slackerror.ErrAppInstall). | ||
| WithMessage("Apps cannot be installed due to project configurations"). | ||
| WithRemediation( | ||
| "Install an app on app settings: %s\nLink an app to this project with %s\nList apps saved with this project using %s", | ||
| style.LinkText("https://api.slack.com/apps"), | ||
| style.Commandf("app link", false), | ||
| style.Commandf("app list", false), | ||
| ). | ||
| WithDetails(slackerror.ErrorDetails{ | ||
| slackerror.ErrorDetail{ | ||
| Code: slackerror.ErrProjectConfigManifestSource, | ||
| Message: "Cannot install apps with manifests sourced from app settings", | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 We no longer want to error on a remote manifest, so this test is removed and replaced with proceeds if manifest.source is remote with the bolt experiment (below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 thought: Once the bolt experiment is removed altogether, we can perhaps remove even more cases!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please!
| func TestAppAddCommand(t *testing.T) { | ||
|
|
||
| testutil.TableTestCommand(t, testutil.CommandTests{ | ||
| "adds a new local app": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Added test for new local apps to complement the existing new deployed app test.
| // When the manifest source is remote and the app exists, we can get the manifest from the the API. | ||
| case manifestSource.Equals(config.ManifestSourceRemote) && app.AppID != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: We need to differentiate between a remote manifest with an app (get manifest from app settings) and a remote manifest without an app (get manifest from local file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ thought: An app.Exists method might be nice here-
docs/reference/experiments.md
Outdated
|
|
||
| * `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` and `slack install` support 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), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I added the slack run in PR #111 but it may have been removed by a rogue docs sync 🥷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukegalbraithrussell Don't sweat it! The docs sync has improved a lot since then, thank to you!
|
|
||
| if !clients.Config.WithExperimentOn(experiment.BoltInstall) { | ||
| if !manifestUpdates && !manifestCreates { | ||
| return app, "", nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Identical to installing a dev app in PR #111
| // 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Identical to installing a dev app in PR #111
internal/pkg/apps/install.go
Outdated
| // TODO: add enterprise ID and user ID to app? See InstallLocalApp. | ||
| // app.EnterpriseID = config.GetContextEnterpriseID(ctx) | ||
| // app.UserID = *authSession.UserID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: While comparing the dev install and this install, I noticed that we set the EnterpriseID and UserID in the other one. I've left a note incase we want to do it here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks EnterpriseID makes a lot of sense to me!
I'm hesitant for the UserID at the moment since we might be using this - alongside the IsDev value - to differentiate "deployed" and "local" apps IIRC 🔍
No blocker for this PR, but the app UserID might be something to inspect more later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @zimeg!
I'll test again on an Enterprise Organization. If the app works properly, I'll hold off on making either of these changes. If it has been working until now, I'd be concerned about a regression. We can follow-up with a separate PR that sets EnterpriseID or something else to align the 2 methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zimeg I've removed the UserID comment so that we don't accidentally implement it later on. I agree, there could be bits of code relying on it to identify a dev app.
docs/reference/experiments.md
Outdated
|
|
||
| * `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` and `slack install` support 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), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * `slack run` and `slack install` support 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), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). | |
| * `slack run` and `slack install` support 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), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). |
assuming this #TODO is still needed to be filled out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew! Thank you, @lukegalbraithrussell! Commit 10966f7 adds the PR number.
| } | ||
|
|
||
| cmd.Flags().StringVar(&addFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) | ||
| cmd.Flags().StringVarP(&addFlags.environmentFlag, "environment", "E", "", "environment of app (local, deployed)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: The slack install command now supports the --environment <local|deployed> values. This flag can be used to by-pass the prompt.
| // TODO(semver:major): Remove this test when the defaulting to deployed is removed. | ||
| "adds a new deployed app when team flag is provided and environment flag is not set": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Test for defaulting to deployed when the team flag is specified and there is no --environment flag.
lukegalbraithrussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs are good!
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks This is looking great so far! 🎉
I'm requesting changes now with a note on the --app flag but will continue a review and testing in the meantime.
Thanks for cleaning adjacent things up with these changes, as always 💌
| } | ||
|
|
||
| func validateManifestForInstall(ctx context.Context, clients *shared.ClientFactory, app types.App, appManifest types.AppManifest) error { | ||
| var token = config.GetContextToken(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 So helpful when walking through the code!
internal/pkg/apps/install.go
Outdated
| // TODO: add enterprise ID and user ID to app? See InstallLocalApp. | ||
| // app.EnterpriseID = config.GetContextEnterpriseID(ctx) | ||
| // app.UserID = *authSession.UserID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks EnterpriseID makes a lot of sense to me!
I'm hesitant for the UserID at the moment since we might be using this - alongside the IsDev value - to differentiate "deployed" and "local" apps IIRC 🔍
No blocker for this PR, but the app UserID might be something to inspect more later!
cmd/app/add.go
Outdated
|
|
||
| // When team flag is provided, default app environment to deployed if not specified. | ||
| // TODO(semver:major): Remove defaulting to deployed and require the environment flag to be set. | ||
| if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" { | |
| if (clients.Config.TeamFlag != "" || clients.Config.AppFlag != "") && addFlags.environmentFlag == "" { |
I'm not certain that this is the right logic, but we might want to use a default value if the --app flag is provided 🔍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗣️ This made me question if --app should accept local or deployed in future versions!
Preferring an --environment flag for that choice and leaving app IDs to --app might bring more straightforward logic here and in other selections, but this isn't a change we ought make now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag combinations can be confusing! 😝
I think we want the original combination of:
if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" {- When
--teamis provided and--environmentis not, then we default to--environment deployed. - When
--appis provided, we resolve the--environmentfromapps.jsonandapps.dev.json. - When there is a mismatched combination, then the
AppSelectwill return an error that the app cannot be found.- Example:
--team T0123 --app A0123whereA0123doesn't belong toT0123
- Example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 question: I noticed the warning below hints at --team needing the --environment flag and was wondering if you think --app should also remove support for "local" or "deployed" in the next semver:major?
🧠 praise: Also, thanks for clearing up these default cases and flag setups alike 🤓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 question: I noticed the warning below hints at
--teamneeding the--environmentflag and was wondering if you think --app should also remove support for "local" or "deployed" in the nextsemver:major?
I won't lie, I thought --app local and --app deployed support was removed in v3.0.0 when we introduced --environment. This should definitely be removed in the next semver:major in favour of --environment. :two-cents:
In the meantime, we'll need to handle that use-case here, I suppose 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 praise: Having a shared environment flag will be so nice. I agree on the continued support until this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾 issue(non-blocking): Hmm I think we might still want to skip this section if an app ID is provided since the environment would be known?
🤖 question: It's not so clear to me if an install can target different environments for a known app. This might not be an issue if so, but I'm wondering what you think.
$ slack install --team tinyspek --app A099WJ22LPK
⚠️ Warning: Default App Environment
App environment is set to deployed when only the --team flag is provided.
The next major version will change this behavior.
When the --team flag is provided, the --environment flag will be required.
Add the '--environment deployed' to avoid breaking changes.
🏠 App Install
Installing "vigilant-shark-290 (local)" app to "Tinyspek"
Finished in 0.4s
📌 note: IMO we should not suggest an app and team flag together once the environment flag becomes more standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾 issue(non-blocking): Hmm I think we might still want to skip this section if an app ID is provided since the environment would be known?
Thanks, after looking over it, I think you're right too. Commit 0e65092 & e28a02e now skips when the --app <id> is provided. If --app <env> then the --environment <env> is set in the lines above.
Can't wait until we can move this logic out of the commands and to a higher-level. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 question: It's not so clear to me if an
installcan target different environments for a known app. This might not be an issue if so, but I'm wondering what you think.
An App ID can only be associated with one environment (1-to-1 relationship), so we don't want to allow the developer to override that. Right now, we error if the --app and --environment flags are used together. That seems the safest. A better approach is that we don't error if --environment matches the saved --app, but atm the error message asks you to remove the --environment flag.
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Thanks for the thorough review @zimeg and for noticing the edge-cases with the --app, --team, and --environment flag combinations.
✨ A few updates since our discussion:
- Display a better warning when the
--teamflag is used and--environment deployis inferred for backwards compability promptIsProduction(...)now reads the--environmentflag properly- Mismatch flag combinations should be handled, such as
--team T0123 --app A0123and this is largely enforced bySelectPrompt(...) - Updated the original description with 2 new videos showcasing additional scripting use-cases
I know it's tough to think through these use-cases, but I'd appreciate your eye on whether we're overlooking additional use-cases! 🙇🏻
| }, | ||
| }) | ||
| } | ||
| clients.Config.SetFlags(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: This was the magic, thanks @zimeg 🪄
| err := clients.Config.Flags.Lookup("environment").Value.Set("deployed") | ||
| if err != nil { | ||
| return ctx, "", types.App{}, err | ||
| } | ||
| clients.Config.Flags.Lookup("environment").Changed = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: We now set the flag value (and mark it as changed), so that the promptIsProduction(...) function can be used to read the --environment flag and optionally prompt for an app environment.
| clients.IO.PrintInfo(ctx, false, "\n"+style.Sectionf(style.TextSection{ | ||
| Emoji: "warning", | ||
| Text: "Warning: Default App Environment", | ||
| Secondary: []string{ | ||
| "App environment is set to deployed when only the --team flag is provided.", | ||
| "The next major version will change this behavior.", | ||
| "When the --team flag is provided, the --environment flag will be required.", | ||
| "Add the '--environment deployed' to avoid breaking changes.", | ||
| }, | ||
| })) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks I'm such a big fan of making this flag combination required! What a good call 🤖 ✨
internal/pkg/apps/install.go
Outdated
| // TODO: add enterprise ID and user ID to app? See InstallLocalApp. | ||
| // app.EnterpriseID = config.GetContextEnterpriseID(ctx) | ||
| // app.UserID = *authSession.UserID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zimeg I've removed the UserID comment so that we don't accidentally implement it later on. I agree, there could be bits of code relying on it to identify a dev app.
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks Thanks so much for keeping the description updated. It makes reviews so much easier 👾
I'm requesting changes here with a side effect of reinstalling apps with the app flag, though it's not so clear to me if the environment flag is also needed here:
$ slack app install --app A0986U6T03A | catOther comments include thoughts and a small suggestion or two, but I'm excited for this to land and it feels close! ✨
| type addCmdFlags struct { | ||
| orgGrantWorkspaceID string | ||
| environmentFlag string | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 thought: IMHO this pattern is so interesting. We might want to encourage it and follow up with changes that bring the app and team and token flags to specific commands soon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 Hmm... we could consider doing that. We may also want to consider --environment to be a global flag, since it should be used in combination with --team 🤔
|
|
||
| manifest := slackYaml.AppManifest | ||
| if slackYaml.IsFunctionRuntimeSlackHosted() { | ||
| manifest := slackManifest.AppManifest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩 praise: The slackManifest including an app manifest is so much more clear than the prior slackYaml!
cmd/app/add.go
Outdated
|
|
||
| // When team flag is provided, default app environment to deployed if not specified. | ||
| // TODO(semver:major): Remove defaulting to deployed and require the environment flag to be set. | ||
| if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧠 question: I noticed the warning below hints at --team needing the --environment flag and was wondering if you think --app should also remove support for "local" or "deployed" in the next semver:major?
🧠 praise: Also, thanks for clearing up these default cases and flag setups alike 🤓
cmd/app/add.go
Outdated
| // Prompt for deployed or local app environment. | ||
| isProductionApp, err := promptIsProduction(ctx, clients) | ||
| if err != nil { | ||
| return ctx, "", types.App{}, err | ||
| } | ||
|
|
||
| // Set the app environment type based on the prompt. | ||
| var appEnvironmentType prompts.AppEnvironmentType | ||
| if isProductionApp { | ||
| appEnvironmentType = prompts.ShowHostedOnly | ||
| } else { | ||
| appEnvironmentType = prompts.ShowLocalOnly | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ issue: This prompt might appear if the app flag is included for an installed app.
🗣️ suggestion: We might want to guard against this case inline and with a test since I'm not sure how else promptIsProduction is used, but returning early might be a solid option too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💟 praise: I like the environment selection before app selection too!
| "errors if manifest.source is remote with the bolt experiment": { | ||
| ExpectedError: slackerror.New(slackerror.ErrAppInstall). | ||
| WithMessage("Apps cannot be installed due to project configurations"). | ||
| WithRemediation( | ||
| "Install an app on app settings: %s\nLink an app to this project with %s\nList apps saved with this project using %s", | ||
| style.LinkText("https://api.slack.com/apps"), | ||
| style.Commandf("app link", false), | ||
| style.Commandf("app list", false), | ||
| ). | ||
| WithDetails(slackerror.ErrorDetails{ | ||
| slackerror.ErrorDetail{ | ||
| Code: slackerror.ErrProjectConfigManifestSource, | ||
| Message: "Cannot install apps with manifests sourced from app settings", | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 thought: Once the bolt experiment is removed altogether, we can perhaps remove even more cases!
| manifestSource: config.ManifestSourceLocal, | ||
| expectedError: slackerror.New(slackerror.ErrSDKHookNotFound), | ||
| }, | ||
| "succeeds if the app exists and the manifest source is remote": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😉 thought: These are clever tests. I think it calls out the importance of error codes in assertions. But seems right here!
Co-authored-by: Eden Zimbelman <[email protected]>
|
Thanks for the thorough eyes @zimeg! I'm seeing a few issues that we'll want to address before this PR is ready. Required changes:
Follow-ups:
|
|
@zimeg Thanks again for the amazing edge-case reviews and sorry about the slow follow-up. I've pushed a few commits to address the 3 tasks in #154 (comment)
I noticed that our |
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwbrooks LGTM! And thank you for the follow ups across these comments 🙏 ✨
The demo cases are great and I agree that more changes to prompts can happen in follow up PRs.
One perhaps issue related to the app flag without an environment flag for a saved "local" app is noted below, but nothing blocking AFAICT. Moving towards the environment flag and deprecated "deploy" and "local" options elsewhere will be such a nice change for the code I hope 🗣️
Please do feel free to merge this if the case below tests alright to you! 🚢 💨
cmd/app/add.go
Outdated
|
|
||
| // When team flag is provided, default app environment to deployed if not specified. | ||
| // TODO(semver:major): Remove defaulting to deployed and require the environment flag to be set. | ||
| if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 praise: Having a shared environment flag will be so nice. I agree on the continued support until this!
| // When the manifest source is remote and the app exists, we can get the manifest from the the API. | ||
| case manifestSource.Equals(config.ManifestSourceRemote) && app.AppID != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👁️🗨️ thought: An app.Exists method might be nice here-
cmd/app/add.go
Outdated
|
|
||
| // When team flag is provided, default app environment to deployed if not specified. | ||
| // TODO(semver:major): Remove defaulting to deployed and require the environment flag to be set. | ||
| if clients.Config.TeamFlag != "" && addFlags.environmentFlag == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾 issue(non-blocking): Hmm I think we might still want to skip this section if an app ID is provided since the environment would be known?
🤖 question: It's not so clear to me if an install can target different environments for a known app. This might not be an issue if so, but I'm wondering what you think.
$ slack install --team tinyspek --app A099WJ22LPK
⚠️ Warning: Default App Environment
App environment is set to deployed when only the --team flag is provided.
The next major version will change this behavior.
When the --team flag is provided, the --environment flag will be required.
Add the '--environment deployed' to avoid breaking changes.
🏠 App Install
Installing "vigilant-shark-290 (local)" app to "Tinyspek"
Finished in 0.4s
📌 note: IMO we should not suggest an app and team flag together once the environment flag becomes more standard.
| // TODO: Move to the promptIsProduction when the prompt is refactored and tested. | ||
| // Validate that the --app flag is not an app ID when the --environment flag is set. | ||
| if types.IsAppID(clients.Config.AppFlag) && addFlags.environmentFlag != "" { | ||
| return ctx, "", types.App{}, slackerror.New(slackerror.ErrMismatchedFlags).WithRemediation("When '--app <app_id>' is set, please do not set the flag --environment.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕊️ praise: This is a pleasant remediation.

CHANGELOG
Summary
This pull request updates the
slack installcommand to create and install new apps that use app settings as the source of truth (remote manifest). It's a continuation of PR #111, which added similar support toslack run.Affected commands:
slack installslack deployslack trigger createFollow-up PRs
--app localand--app deployed/--app <app-id>and--app <env>validation topromptIsProductionand add test coverage.Reviewers
Each use-case includes a collapsed section with manual steps.
Deno SDK -
slack install2025-07-11-install-on-remote-manifest-deno-install.mov
Manual Test Steps
Deno SDK -
slack deploy2025-07-11-install-on-remote-manifest-deno-deploy.mov
Manual Test Steps
Deno SDK -
slack trigger create2025-07-11-install-on-remote-manifest-deno-trigger.mov
Manual Test Steps
Bolt JS -
slack install2025-07-11-install-on-remote-manifest-bolt-install.mov
Manual Test Steps
Bolt JS -
slack deploy2025-07-11-install-on-remote-manifest-bolt-install.mov
Manual Test Steps
Scripting & Flags -
slack install --team T0123 --environment local2025-07-11-install-on-remote-manifest-flags.mov
Manual Test Steps
Scripting & Flags -
slack install --team T01232025-07-24-install-team.mov
Manual Test Steps
Scripting & Flags - Flag Mismatch
--app,--team, &--environment2025-07-24-install-team-mismatch.mov
Manual Test Steps
Scripting & Flags - Support
--app localand--app deployedWorking example:
https://github.com/user-attachments/assets/dcc19355-4b65-42ff-a115-56cd57610598
Mismatch errors:
https://github.com/user-attachments/assets/dea85d8d-8751-4aac-a571-f3d306b2dec1
Manual Test Steps
Scripting & Flags - Support
--app A123Working example:
https://github.com/user-attachments/assets/42caf961-2200-4db2-b683-c2c43ce16c35
Mismatch errors:
2025-08-07-install-w-app-id-mismatch.mov
Manual Test Steps
Requirements