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 12 commits into
Conversation
srtaalej
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
+ Coverage 71.64% 71.66% +0.01%
==========================================
Files 226 226
Lines 19154 19178 +24
==========================================
+ Hits 13723 13744 +21
- Misses 4220 4222 +2
- Partials 1211 1212 +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
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
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