feat(create): fetch remote manifest after linking app during create#585
feat(create): fetch remote manifest after linking app during create#585srtaalej wants to merge 25 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
- Coverage 71.71% 71.67% -0.04%
==========================================
Files 226 227 +1
Lines 19182 19212 +30
==========================================
+ Hits 13757 13771 +14
- Misses 4213 4230 +17
+ Partials 1212 1211 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolve conflicts by integrating main's stricter --app/--environment validation with the remote manifest fetch feature. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Thanks for sharing these changes 👾 ✨
I'm leaving a few comments on a first run experience around prompts and earlier errors. Other comments are on code organization with thoughts around the difference between create and link logic that might be interesting to clear up ✂️ 📠
Testing is solid for me too but I leave a note around escaped values that are unsettling to me. I'm unsure what caused that 👻
| // fetchAndWriteRemoteManifest fetches the app manifest from remote settings and writes it to the project. | ||
| func fetchAndWriteRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token, appID, projectPath string) error { | ||
| slackYaml, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data, err := json.MarshalIndent(slackYaml.AppManifest, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data = append(data, '\n') | ||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| return afero.WriteFile(clients.Fs, manifestPath, data, 0644) | ||
| } | ||
|
|
| clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{ | ||
| Text: "Could not fetch the remote app manifest", | ||
| Secondary: []string{ | ||
| fetchErr.Error(), | ||
| "The template manifest was kept unchanged", | ||
| }, | ||
| })) |
There was a problem hiding this comment.
There was a problem hiding this comment.
hmm I'm hesitant because the create+--app command doesn't necessarily fail if fetching remote manifest fails. the app is still created and linked successfully. only one part of the command fails in this case so maybe it would be better to provide a way to try again in follow up PRs?
or simply telling users to run slack manifest info --source remote and copy that into manifest.json 🤔
There was a problem hiding this comment.
👁️🗨️ thought: I'm worried I'd miss warnings in CI and overwrite an existing app manifest for "A001" with these commands if the manifest step of "create" fails:
$ slack create my-app -t slack-samples/bolt-js-blank-template -a A001 -E deployed
$ slack deployRecommending to save the outputs of manifest info to a file is a good remediation!
| LinkAppFooterSection(ctx, clients, app) | ||
|
|
||
| return nil | ||
| return auth, nil |
There was a problem hiding this comment.
🔗 question: Instead of returning an auth to calling functions that's often ignored, would setting up the manifest as part of the link command if no apps are saved make sense?
👾 ramble: I'm wanting to avoid mixing concepts with the create command which should be from a project template I think while link is seeming more app specific.
| if err != nil { | ||
| return err | ||
| } | ||
| data, err := json.MarshalIndent(slackYaml.AppManifest, "", " ") |
There was a problem hiding this comment.
🔬 note: I'm finding some values are escaped but this hasn't caused an error when testing the custom step template:
- "type": "slack#/types/user_id",
+ "type": "slack#\/types\/user_id",| } | ||
| data = append(data, '\n') | ||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||
| return afero.WriteFile(clients.Fs, manifestPath, data, 0644) |
There was a problem hiding this comment.
🏁 suggestion: We should save the hash alongside this to avoid a confusing prompt of a changed manifest on the first "run" command:
slack-cli/internal/pkg/apps/install.go
Lines 167 to 176 in f984c33
$ cd my-test
$ slack run
📚 App Manifest
Manifest values for this app are overwritten on reinstall
┃ Overwrite manifest on app settings with the project's manifest file?
┃ Yes
┃ ❱ No
mwbrooks
left a comment
There was a problem hiding this comment.
Answers to Open Questions:
Open question: should we set manifest.source to remote in .slack/config.json after writing the fetched manifest? Currently left as local so the user "owns" the file going forward.
It's a good question - I think we should not change the manifest source. The main reason is that this keeps the existing functionality and it's unclear whether the developer would prefer App Settings or manifest.json when creating an app locally from App Settings.
So, since there's no clear answer, I'd rather keep the functionality unchanged. Instead, we can continue to move forward on the 2-way sync solution to remove this issue entirely.
manifest.ts/.js (Deno SDK) projects are not supported
Totally fine to not support manifest.ts/.js. This is a Deno SDK only feature and App Settings isn't offering Deno SDK creation journeys afaik.
mwbrooks
left a comment
There was a problem hiding this comment.
💬 @srtaalej This is shaping up really nice! I've left a suggestion on how we can tighten up the LinkExistingApp function to return the auth while also improving how it currently sets app. While it feels like a larger change, it turns out to be a similar line count and should help align this PR around @zimeg's feedback.
| // link an existing app and additional information is included in the header. | ||
| // The shouldConfirm option is encouraged for third-party callers. | ||
| func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (err error) { | ||
| func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (_ *types.SlackAuth, err error) { |
There was a problem hiding this comment.
suggestion: We should consider returning a single prompts.SelectedApp (contains an app and auth) instead of mixing a pointer out-parameter (app *types.App) with the new returned types.SlackAuth.
The app parameter is currently an "out-param" dressed up as an "in-param" - it's always overwritten at L171 (*app, auth, err = promptExistingApp(...)). Every caller passes a throwaway &types.App{}.
After this PR, app comes out via the pointer while auth comes out via the return value - two different output mechanisms for two values produced at the same call site. This makes the function confusing and brittle.
prompts.SelectedApp already pairs App + Auth and is the exact shape this function produces. Also, cmd/app already imports internal/prompts, so no new dependency.
Suggested signature:
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, shouldConfirm bool) (prompts.SelectedApp, error)A zero SelectedApp{} (i.e. selected.App.AppID == "") signals "user declined at the confirm prompt" - which is what create.go already checks on L243.
Caller changes:
LinkCommandRunE(this file): drop theapp *types.Appparam; call_, err := LinkExistingApp(ctx, clients, false). Also remove the throwawayapp := &types.App{}inNewLinkCommand.cmd/project/init.go:113:_, err = app.LinkExistingApp(ctx, clients, true)cmd/project/create.go:232:selected, linkErr := app.LinkExistingApp(ctx, clients, false)thenif selected.App.AppID != "" { fetchAndWriteRemoteManifest(ctx, clients, selected.Auth.Token, selected.App.AppID, absProjectPath) }— the existingauth != nil && linkedApp.AppID != ""guard collapses to one condition.
Diff size is comparable to the current PR, no callers need to manufacture an empty App{}, and the API answers the obvious question: yes, this function returns a selected app.
There was a problem hiding this comment.
thank you for this clarification @mwbrooks! I agree the LinkExistingApp is a bit messy 🤔 since i have another pr that also makes changes to LinkExistingApp, i feel like i should leave that refactor to a follow-up PR to avoid more conflicts. for now i have taken out the auth param i introduced and instead get it from:
@create.go
if linkedApp.AppID != "" {
auth, err := clients.Auth().AuthWithTeamID(ctx, linkedApp.TeamID)
if err != nil { return err }
fetchErr := manifest.FetchAndWriteRemoteManifest(ctx, clients, auth.Token, ...)
...
so that link.go remains unchanged. what do you think of this approach?
There was a problem hiding this comment.
@srtaalej I think your approach to manually get the auth param works and keeps the impact low. So feel free to run with that!
UpdateThe diff is much smaller now:
Follow-ups
|
| } | ||
| fetchErr := manifest.FetchAndWriteRemoteManifest(ctx, clients, auth.Token, linkedApp.AppID, absProjectPath) | ||
| if fetchErr != nil { | ||
| clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{ |
There was a problem hiding this comment.
left a comment about this silent fail 🪂 #585 (comment) @zimeg
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Super nice to find earlier PRs building up to this feature! 💡 ✨
I'm leaving a few comments after reviewing this that I'm curious to discuss perhaps with the last point:
🌩️ Missing outputs: It's not clear that the template manifest is overwritten based on the output. I think a line or so toward this might be helpful.
🍵 Hidden errors: Erroring when writing the remote manifest can lead to unexpected cases in CI and I share when this might happen and that earlier returns are favorable to debug IMO.
🌈 Saving to file: The suggestion you share to remediate these errors is great and I'm curious now if we extend the manifest info command to support such, that we might call here?
$ slack manifest info --app A001 --to-file manifest.json
📸 Here's the output referenced!
$ slack create my-test -t slack-samples/bolt-js-starter-template --app A07V06USK0X --team T038J6TH5PF --environment local
🗣️ If the manifest info approach seems alright I think it'd be a good separate PR that'd make this one even more focused! These are all promising changes.
| var buf bytes.Buffer | ||
| encoder := json.NewEncoder(&buf) | ||
| encoder.SetEscapeHTML(false) | ||
| encoder.SetIndent("", " ") | ||
| if err := encoder.Encode(slackYaml.AppManifest); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🔭 question: Can we reuse the stringifier in adjacent package?
slack-cli/internal/goutils/json.go
Lines 62 to 74 in 19cfb59
There was a problem hiding this comment.
🧪 todo: Let's add unit tests alongside internal packages for more strict checks!
| clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{ | ||
| Text: "Could not fetch the remote app manifest", | ||
| Secondary: []string{ | ||
| fetchErr.Error(), | ||
| "The template manifest was kept unchanged", | ||
| }, | ||
| })) |
There was a problem hiding this comment.
👁️🗨️ thought: I'm worried I'd miss warnings in CI and overwrite an existing app manifest for "A001" with these commands if the manifest step of "create" fails:
$ slack create my-app -t slack-samples/bolt-js-blank-template -a A001 -E deployed
$ slack deployRecommending to save the outputs of manifest info to a file is a good remediation!
| ) | ||
|
|
||
| // FetchAndWriteRemoteManifest fetches the app manifest from remote settings and writes it to the project. | ||
| func FetchAndWriteRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token, appID, projectPath string) error { |
There was a problem hiding this comment.
| func FetchAndWriteRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token, appID, projectPath string) error { | |
| func SetManifestLocal(ctx context.Context, clients *shared.ClientFactory, slackYaml types.SlackYaml) error { |
🧮 thought: This might be nice to pair with other app manifest methods although a best file structure isn't clear to me right now:
- GetManifestRemote
- GetManifestLocal
- SetManifestLocal: New!
There was a problem hiding this comment.
📝 note: This suggestion might go well with the idea to extend manifest info too?
| hash, err := clients.Config.ProjectConfig.Cache().NewManifestHash(ctx, slackYaml.AppManifest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return clients.Config.ProjectConfig.Cache().SetManifestHash(ctx, appID, hash) |
There was a problem hiding this comment.
🏆 praise: Thanks for including this!
🦔 thought: I'm now curious if this would be better as part of the app link command? We might pull the manifest to write a hash when linking a specific app which can improve the link experience outside of create command?
Changelog
slack create --appnow fetches the app's manifest from remote settings and writes it to the project'smanifest.json, replacing the template's generic manifest with the actual app configuration.Summary
Follow-up to #565. After linking an existing app during
slack create --app --template, the CLI now fetches the remote manifest from App Settings and overwrites the template'smanifest.jsonwith it.This also updates
LinkExistingAppto return (*types.SlackAuth, error) so callers can use the auth token for subsequent API calls (like manifest fetching).Design decisions:
.slack/config.json— the remote content is written locallymanifest.jsonis overwritten (notmanifest.ts/.jswhich are code files in Deno projects)Testing
Manual:
make build./bin/slack create my-test -t slack-samples/bolt-js-starter-template --app <real-app-id> --environment localmy-test/manifest.jsoncontains the remote app's manifestNotes 🔴
.slack/config.jsonafter writing the fetched manifest? Currently left as local so the user "owns" the file going forward.manifest.ts/.js(Deno SDK) projects are not supported⌛ not merging until #598 is merged because of conflicts
Requirements