-
Notifications
You must be signed in to change notification settings - Fork 25
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
Changes from 14 commits
7578b6e
46be447
7678bb4
9847d3c
10966f7
fda7732
aa42052
9b04aa7
521ee30
886568f
12d89f4
97fe3f0
edbcaf9
976d285
d62e7c6
013ad0d
1e2bd54
701a794
9e757b1
c868e1b
69e3284
0e65092
e28a02e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,7 @@ var appSelectPromptFunc = prompts.AppSelectPrompt | |||||
|
|
||||||
| type addCmdFlags struct { | ||||||
| orgGrantWorkspaceID string | ||||||
| environmentFlag string | ||||||
| } | ||||||
|
|
||||||
| var addFlags addCmdFlags | ||||||
|
|
@@ -56,10 +57,11 @@ func NewAddCommand(clients *shared.ClientFactory) *cobra.Command { | |||||
| Example: style.ExampleCommandsf([]style.ExampleCommand{ | ||||||
| {Command: "app install", Meaning: "Install a production app to a team"}, | ||||||
| {Command: "app install --team T0123456", Meaning: "Install a production app to a specific team"}, | ||||||
mwbrooks marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| {Command: "app install --team T0123456 --environment local", Meaning: "Install a local dev app to a specific team"}, | ||||||
| }), | ||||||
| PreRunE: func(cmd *cobra.Command, args []string) error { | ||||||
| ctx := cmd.Context() | ||||||
| return preRunAddCommand(ctx, clients) | ||||||
| return preRunAddCommand(ctx, clients, cmd) | ||||||
| }, | ||||||
| RunE: func(cmd *cobra.Command, args []string) error { | ||||||
| ctx := cmd.Context() | ||||||
|
|
@@ -72,51 +74,73 @@ func NewAddCommand(clients *shared.ClientFactory) *cobra.Command { | |||||
| } | ||||||
|
|
||||||
| cmd.Flags().StringVar(&addFlags.orgGrantWorkspaceID, cmdutil.OrgGrantWorkspaceFlag, "", cmdutil.OrgGrantWorkspaceDescription()) | ||||||
| cmd.Flags().StringVarP(&addFlags.environmentFlag, "environment", "E", "", "environment of app (local, deployed)") | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: The |
||||||
|
|
||||||
| return cmd | ||||||
| } | ||||||
|
|
||||||
| // preRunAddCommand confirms an app is available for installation | ||||||
| func preRunAddCommand(ctx context.Context, clients *shared.ClientFactory) error { | ||||||
| func preRunAddCommand(ctx context.Context, clients *shared.ClientFactory, cmd *cobra.Command) error { | ||||||
| err := cmdutil.IsValidProjectDirectory(clients) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
| if !clients.Config.WithExperimentOn(experiment.BoltFrameworks) { | ||||||
| return nil | ||||||
| } | ||||||
| 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", | ||||||
| }, | ||||||
| }) | ||||||
| } | ||||||
|
Comment on lines
-88
to
-107
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪓 Removing this section that prevents the |
||||||
| clients.Config.SetFlags(cmd) | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: This was the magic, thanks @zimeg 🪄 |
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // RunAddCommand executes the workspace install command, prints output, and returns any errors. | ||||||
| func RunAddCommand(ctx context.Context, clients *shared.ClientFactory, selection *prompts.SelectedApp, orgGrantWorkspaceID string) (context.Context, types.InstallState, types.App, error) { | ||||||
| if selection == nil { | ||||||
| selected, err := appSelectPromptFunc(ctx, clients, prompts.ShowHostedOnly, prompts.ShowAllApps) | ||||||
| // 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 == "" { | ||||||
|
||||||
| 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.
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.
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 🤖 ✨
Outdated
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!
Outdated
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.

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
appandteamandtokenflags 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
--environmentto be a global flag, since it should be used in combination with--team🤔