feat: add --app flag support to slack create for linking existing apps#565
feat: add --app flag support to slack create for linking existing apps#565srtaalej wants to merge 15 commits into
Conversation
srtaalej
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
+ Coverage 71.64% 71.68% +0.04%
==========================================
Files 226 226
Lines 19154 19184 +30
==========================================
+ Hits 13723 13753 +30
- Misses 4220 4221 +1
+ Partials 1211 1210 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Awesome changes going on here! 🎁
I'm leaving a handful of comments around refactoring logic into adjacent places. Hoping that we can compose commands overall and avoid adding too much to create for ongoing iteration.
Two notable changes to the experience that I'll call out here include:
- Accepting the environment flag alongside a default
- Skipping the name prompt when an existing app ID is provided
Quite excited for what this hopes to unlock 🔏
| // fetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | ||
| func fetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | ||
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | ||
| if err != nil { | ||
| return types.SlackYaml{}, slackerror.New(slackerror.ErrInvalidManifest). | ||
| WithMessage("Failed to fetch manifest for app %s", appID) | ||
| } | ||
| return manifest, nil | ||
| } |
There was a problem hiding this comment.
| // fetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | |
| func fetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | |
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | |
| if err != nil { | |
| return types.SlackYaml{}, slackerror.New(slackerror.ErrInvalidManifest). | |
| WithMessage("Failed to fetch manifest for app %s", appID) | |
| } | |
| return manifest, nil | |
| } |
🪓 suggestion: Let's inline this! I think the error returned might sometimes be different from invalid manifest that we might want to surface
| // writeManifestToProject writes the fetched manifest JSON to the project directory. | ||
| func writeManifestToProject(fs afero.Fs, projectPath string, manifest types.SlackYaml) error { | ||
| manifestData, err := json.MarshalIndent(manifest.AppManifest, "", " ") | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to serialize app manifest") | ||
| } | ||
|
|
||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| if err := afero.WriteFile(fs, manifestPath, append(manifestData, '\n'), 0644); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to write manifest to project") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛻 suggestion: Let's move this logic to internal/app/manifest for adjacent changes of #543
| // linkAppToProject saves the app to the project's apps JSON file. | ||
| // Defaults to local/dev unless the manifest explicitly uses a hosted runtime. | ||
| func linkAppToProject(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, appID string, manifest types.SlackYaml) error { | ||
| app := types.App{ | ||
| AppID: appID, | ||
| TeamID: auth.TeamID, | ||
| TeamDomain: auth.TeamDomain, | ||
| EnterpriseID: auth.EnterpriseID, | ||
| } | ||
|
|
||
| if manifest.IsFunctionRuntimeSlackHosted() { | ||
| return clients.AppClient().SaveDeployed(ctx, app) | ||
| } | ||
| app.IsDev = true | ||
| app.UserID = auth.UserID | ||
| return clients.AppClient().SaveLocal(ctx, app) | ||
| } |
There was a problem hiding this comment.
🔭 question: Can we reuse logic of the app link command? We perhaps might change outputs but I'm hoping we move toward focused and atomic commands that perhaps compose!
There was a problem hiding this comment.
👾 issue: I'm concerned of the forced default local app here. The same CI example I share earlier is for a production app and I don't have immediate option to "deploy" the right app after using this:
$ slack create --app A0582JYKGB1 --template zimeg/slacks --branch snaek --name snaek --force
🌠 suggestion: We might want to use the --environment flag to decide this? I still think a default "local" makes sense - CI should be explicit!
| WithMessage("The --app flag requires the --template flag when used with create") | ||
| } | ||
|
|
||
| // Fail fast: resolve auth and fetch manifest before creating the project |
There was a problem hiding this comment.
🪬 thought: Related to comments of logic moved to internal I'm curious if we can move this check too? I understand a mismatched app ID will cause error but I don't think we should prompt for name when the --app flag is used...
🐮 ramble: For example a minimal example seems excessive for CI use case:
$ slack create --app A0582JYKGB1 --template zimeg/slacks --branch snaek --name snaek --force
There was a problem hiding this comment.
🧂 ramble: Similar comment as below I wonder if this is logic can be passed as createArgs or perhaps afterwards to the link command?
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Leaving a few more comments around logic that we might want to separate between project create and app link command 🔭
To me it's seeming more that these commands should be sequenced so the --app and --environment flags are passed through the "link" command after the "create" command clones the template. I'm unsure of right scope for this but notice a few improvements happening:
- Finding saved authentication for a provided
linkapp 🎁 - Copying the existing app manifest from upstream app settings ⚙️
- Saving the provided app ID from
createflags 🏁
Am requesting approval to hope we can reuse more command logic and think these enhancements might be alright to break into multiple changesets if the notes above seem right?
| WithMessage("The --subdir flag requires the --template flag") | ||
| } | ||
|
|
||
| // --app requires --template (Mode 2 deferred) |
There was a problem hiding this comment.
👁️🗨️question: Which mode 2 in this case?
| // FetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | ||
| func FetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | ||
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | ||
| if err != nil { | ||
| return types.SlackYaml{}, slackerror.Wrap(err, slackerror.ErrInvalidManifest). | ||
| WithMessage("Failed to fetch manifest for app %s", appID) | ||
| } | ||
| return manifest, nil | ||
| } |
There was a problem hiding this comment.
🪓 suggestion: Let's inline this! I think the error returned might sometimes be different from invalid manifest that we might want to surface
| // FetchRemoteManifest retrieves the app manifest from the platform via apps.manifest.export. | |
| func FetchRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token string, appID string) (types.SlackYaml, error) { | |
| manifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | |
| if err != nil { | |
| return types.SlackYaml{}, slackerror.Wrap(err, slackerror.ErrInvalidManifest). | |
| WithMessage("Failed to fetch manifest for app %s", appID) | |
| } | |
| return manifest, nil | |
| } |
| // SaveAppToProject writes the linked app to the project's apps JSON file, | ||
| // checking for conflicts before saving unless --force is set. | ||
| func SaveAppToProject(ctx context.Context, clients *shared.ClientFactory, app types.App) error { | ||
| deploy, err := clients.AppClient().GetDeployed(ctx, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| local, err := clients.AppClient().GetLocal(ctx, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| switch app.IsDev { | ||
| case true: | ||
| if clients.Config.ForceFlag || (local.IsNew() && deploy.AppID != app.AppID) { | ||
| return clients.AppClient().SaveLocal(ctx, app) | ||
| } | ||
| case false: | ||
| if clients.Config.ForceFlag || (deploy.IsNew() && local.AppID != app.AppID) { | ||
| return clients.AppClient().SaveDeployed(ctx, app) | ||
| } | ||
| } | ||
| return slackerror.New(slackerror.ErrAppFound). | ||
| WithMessage("A saved app was found and cannot be overwritten"). | ||
| WithRemediation("Remove the app from this project or try again with %s", style.Bold("--force")) | ||
| } |
There was a problem hiding this comment.
🦠 suggestion: This is duplicate to app link implementation we might not want to duplicate this?
Lines 283 to 307 in dc30dc3
| // WriteManifestToProject writes the fetched manifest JSON to the project directory. | ||
| func WriteManifestToProject(fs afero.Fs, projectPath string, manifest types.SlackYaml) error { | ||
| manifestData, err := json.MarshalIndent(manifest.AppManifest, "", " ") | ||
| if err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to serialize app manifest") | ||
| } | ||
|
|
||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| if err := afero.WriteFile(fs, manifestPath, append(manifestData, '\n'), 0644); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrProjectFileUpdate). | ||
| WithMessage("Failed to write manifest to project") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗳️ suggestion: We might want to make this internal/app/manifest.go I think we want to move away from internal/pkg/....... ongoing
| // ResolveAuthForApp finds an authenticated workspace that has access to the given app ID. | ||
| func ResolveAuthForApp(ctx context.Context, clients *shared.ClientFactory, appID string) (types.SlackAuth, error) { | ||
| if clients.Config.TokenFlag != "" { | ||
| auth, err := clients.Auth().AuthWithToken(ctx, clients.Config.TokenFlag) | ||
| if err != nil { | ||
| return types.SlackAuth{}, slackerror.Wrap(err, slackerror.ErrNotAuthed) | ||
| } | ||
| return auth, nil | ||
| } | ||
|
|
||
| allAuths, err := clients.Auth().Auths(ctx) | ||
| if err != nil { | ||
| return types.SlackAuth{}, slackerror.Wrap(err, slackerror.ErrNotAuthed) | ||
| } | ||
|
|
||
| if len(allAuths) == 0 { | ||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrNotAuthed). | ||
| WithMessage("No workspaces connected"). | ||
| WithRemediation("Run %s to sign in to a workspace that has access to app %s", style.Commandf("login", false), appID) | ||
| } | ||
|
|
||
| if clients.Config.TeamFlag != "" { | ||
| for i := range allAuths { | ||
| if allAuths[i].TeamID == clients.Config.TeamFlag || allAuths[i].TeamDomain == clients.Config.TeamFlag { | ||
| if _, err := clients.API().GetAppStatus(ctx, allAuths[i].Token, []string{appID}, allAuths[i].TeamID); err == nil { | ||
| return allAuths[i], nil | ||
| } | ||
| } | ||
| } | ||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrTeamNotFound). | ||
| WithMessage("The specified team does not have access to app %s", appID). | ||
| WithRemediation("Run %s to sign in to the workspace that owns this app", style.Commandf("login", false)) | ||
| } | ||
|
|
||
| for i := range allAuths { | ||
| if _, err := clients.API().GetAppStatus(ctx, allAuths[i].Token, []string{appID}, allAuths[i].TeamID); err == nil { | ||
| return allAuths[i], nil | ||
| } | ||
| } | ||
|
|
||
| return types.SlackAuth{}, slackerror.New(slackerror.ErrAppNotFound). | ||
| WithMessage("No authenticated workspace has access to app %s", appID). | ||
| WithRemediation("Run %s to sign in to the workspace that owns this app", style.Commandf("login", false)) | ||
| } |
There was a problem hiding this comment.
💾 thought: This might be something we use to replace the following:
Lines 211 to 240 in aceb7a4
There was a problem hiding this comment.
👾 ramble: Am leaving comments in hopes that these additions can be reused more as:
- Run the
createcommand- If "--app" flag then run the
linkcommand - No "--app" flag continues without change
- If "--app" flag then run the
| WithMessage("The --app flag requires the --template flag when used with create") | ||
| } | ||
|
|
||
| // Fail fast: resolve auth and fetch manifest before creating the project |
There was a problem hiding this comment.
🧂 ramble: Similar comment as below I wonder if this is logic can be passed as createArgs or perhaps afterwards to the link command?
|
@zimeg thanks for re-review! i agree these should be separate changesets 🤔 your comments definitely clarified the command flow! |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Thanks for breaking this into multiple changes 🪬 ✨
The comments I'm leaving are much more focused and I think we can land this in separated changes to build confidence throughout. Right now I comment on testing and handling edge cases more before building on this more 🔭
Overall an exciting experience to start with an existing app! 🎁
| var createAppNameFlag string | ||
| var createListFlag bool | ||
| var createSubdirFlag string | ||
| var createEnvironmentFlag string |
There was a problem hiding this comment.
🧮 quibble: For alphabetical order?
| {Command: "create my-project -t slack-samples/deno-hello-world", Meaning: "Start a new project from a specific template"}, | ||
| {Command: "create --name my-project", Meaning: "Create a project named 'my-project'"}, | ||
| {Command: "create my-project -t org/monorepo --subdir apps/my-app", Meaning: "Create from a subdirectory of a template"}, | ||
| {Command: "create my-project -t slack-samples/bolt-js-starter-template --app A0123456789", Meaning: "Create from template and link to an existing app"}, |
There was a problem hiding this comment.
🪬 suggestion: Including the --environment flag might be meaningful with this example to avoid retries in CI setup or experiments avoiding prompts?
There was a problem hiding this comment.
suggestion: Instead, how about we add a CI/Scripting example and allow the above example to stay focused on creating a project with an existing app ID.
| // --environment requires --app | ||
| if cmd.Flags().Changed("environment") && !appFlagProvided { | ||
| return slackerror.New(slackerror.ErrMismatchedFlags). | ||
| WithMessage("The --environment flag requires the --app flag when used with create") | ||
| } |
There was a problem hiding this comment.
🔬 thought: We might guard against unexpected --environment values with this check or perhaps reset all changes if link command errors? I find template remains in an incomplete state after this command:
$ slack create --template slack-samples/bolt-js-starter-template --app A0B93LJK24A --name asdf --environment deploy --team slackbox-ez
🦠 ramble: The issue here is "deploy" instead of "deployed" I believe!
| }, | ||
| }, func(cf *shared.ClientFactory) *cobra.Command { | ||
| return NewCreateCommand(cf) | ||
| }) |
There was a problem hiding this comment.
🧪 suggestion: Adding cases to cover the expected happy paths might be meaningful to guarantee the create command keeps support for these paths as underlined commands change
There was a problem hiding this comment.
If I understand correctly, Eden would prefer that we don't mock the LinkFunc (old pattern that's becoming an anti-pattern for us) and instead have our tests call into the real link code to verify that the app is correctly linked.
There was a problem hiding this comment.
Commit 6533815 removes the LinkFunc stub and mock. It now uses app.LinkExistingApp(...) in the implementation and tests.
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
| // --app requires --template | ||
| appFlagProvided := clients.Config.AppFlag != "" && types.IsAppID(clients.Config.AppFlag) |
There was a problem hiding this comment.
minor: --app local will silently skip linking because it's not an App ID. While it's correct that we don't want to support --app local and --app deployed when creating an app, we should display an error immediately when it's not an App ID.
There was a problem hiding this comment.
Commit c9f5bbf now displays an error during the creation if a non-App-ID is provided. For example, --app local or --app deployed.
| originalDir, _ := clients.Os.Getwd() | ||
| if err := os.Chdir(absProjectPath); err != nil { | ||
| return slackerror.Wrap(err, slackerror.ErrAppDirectoryAccess) | ||
| } | ||
| linkErr := LinkFunc(ctx, clients, &types.App{}, false) | ||
| _ = os.Chdir(originalDir) |
There was a problem hiding this comment.
minor: We should handle the error returned by clients.Os.Getwd() and handle the error / add a deferr to `os.Chdir(origianlDir).
There was a problem hiding this comment.
Commit 7564232 now handles errors when getting or changing the working directory. It also defers restoring the original working directory, so that it's restored safely during errors.
| LinkFunc = func(ctx context.Context, clients *shared.ClientFactory, a *types.App, shouldConfirm bool) error { | ||
| linkCalled = true | ||
| assert.False(t, shouldConfirm) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
note: We should be backing up the original LinkFunc and resetting it on each test. Right now the new func(...) will carry-over to future tests.
There was a problem hiding this comment.
Commit 6533815 removes this code and now uses the real link function.
| }, | ||
| }, func(cf *shared.ClientFactory) *cobra.Command { | ||
| return NewCreateCommand(cf) | ||
| }) |
There was a problem hiding this comment.
If I understand correctly, Eden would prefer that we don't mock the LinkFunc (old pattern that's becoming an anti-pattern for us) and instead have our tests call into the real link code to verify that the app is correctly linked.
The --app flag silently accepted environment-style values (local, deployed) and lowercase typos because the gate combined non-empty and IsAppID into a single boolean. The project would scaffold and the link step would be skipped without any error. Validate --app explicitly: when it is set but not an app ID, return ErrInvalidAppFlag immediately so users see a clear error rather than a silent skip.
Three issues in the cwd dance after creating a project with --app: 1. The Getwd error was discarded, so a Getwd failure left originalDir empty and silently broke the restore. 2. The os.Chdir restore was inline, not deferred, so a panic in LinkExistingApp would skip it and leave the user's process in the project subdir. 3. The os.Chdir restore error was discarded. Capture the Getwd error and return it, defer the restore so it runs on panic, and stop collapsing linkErr through a temporary variable. Mirrors the pattern already used in internal/pkg/create/create.go.
Drop the var LinkFunc = app.LinkExistingApp test seam and call the real function directly. The two happy-path tests now mock LinkExistingApp's underlying dependencies (Auth.Auths, IO prompts, API.GetAppStatus, the project config and manifest) so they verify the app is actually persisted via cm.AppClient.GetDeployed/GetLocal — mirroring the test pattern in cmd/app/link_test.go. This also resolves the leak where the swapped LinkFunc was not restored between tests.
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
✅ Thanks for putting together this PR @srtaalej. I'm throwing an approval on because I think all of the feedback is handled.
💻 I've added 3 new commits based on the remaining feedback:
- c9f5bbf - fix: validate --app value as an app ID in create
- 7564232 - fix: restore working directory safely after linking in create
- 6533815 - refactor: remove LinkFunc indirection from create tests
🧪 Local testing looks good. However, I had to add a --team <team_id> flag in order to skip the team selection prompt. This is something that we'll want to improve in a future PR @srtaalej.
🧠 I think a future PR should also strive to clean up the output of the create experience when --app is provided. The linking output looks noisy and confusing with multiple App Link and App Manifest headers.
📝 @srtaalej A few follow-up tasks that you should note are:
- App Settings Creation should include the
--team T0123and--environment localflags in order to skip the prompts. - Improve the output experience for
createwhen--appis provided. - Discuss updating the CLI to not require the
--teamflag unless there are multiple auth'd teams with access to the App ID. This would require discovering the Team ID that has access to the App ID. - Discuss updating the CLI to not require the
--environment localflag for the create command. A default could belocaland advanced developers could specify it if they want deployed.
3 participants
Changelog
slack createnow accepts the--appflag alongside--templateto scaffold a project and automatically link it to an existing app viaapp link.Summary
This PR adds
--app [ID]and--environmentflag support toslack create. When used with--template, the CLI will:app linkflow to save the app ID to the projectThe
--environmentflag (local/deployed) is passed through toapp linkto determine which file the app is saved to. If omitted, the user is prompted.Flag validation:
--appwithout--templatereturns an error--environmentwithout--appreturns an errorExample
slack create my-project -t slack-samples/bolt-js-starter-template --app A0123456789 --environment localTesting
Manual verification:
./bin/slack create my-project -t slack-samples/bolt-js-starter-template --app <real-app-id> --environment local.slack/apps.dev.jsoncontains linked app with correct team/app IDs--appwithout--templatereturns helpful error--environmentwithout--appreturns helpful errorslack create(no--app) works unchangedNotes
Requirements