-
Notifications
You must be signed in to change notification settings - Fork 35
feat(create): fetch remote manifest after linking app during create #585
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
base: main
Are you sure you want to change the base?
Changes from all commits
0d8bfd3
da2eb75
e57f7d5
aceb7a4
906346a
8128d5e
315c9bb
3177aa1
6d6749a
42782a3
3d642ca
f984c33
3396d78
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 |
|---|---|---|
|
|
@@ -97,7 +97,7 @@ func LinkCommandRunE(ctx context.Context, clients *shared.ClientFactory, app *ty | |
| // Add empty line between executed command and first output | ||
| clients.IO.PrintInfo(ctx, false, "") | ||
|
|
||
| err = LinkExistingApp(ctx, clients, app, false) | ||
| _, err = LinkExistingApp(ctx, clients, app, false) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -130,7 +130,7 @@ func LinkAppHeaderSection(ctx context.Context, clients *shared.ClientFactory, sh | |
| // When shouldConfirm is true, a confirmation prompt will ask the user if they want to | ||
| // 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) { | ||
| // Header section | ||
| LinkAppHeaderSection(ctx, clients, shouldConfirm) | ||
|
|
||
|
|
@@ -139,21 +139,21 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty | |
| proceed, err := clients.IO.ConfirmPrompt(ctx, LinkAppConfirmPromptText, true) | ||
| if err != nil { | ||
| clients.IO.PrintDebug(ctx, "Error prompting to add an existing app: %s", err) | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| // Add newline to match the trailing newline inserted from the footer section | ||
| clients.IO.PrintInfo(ctx, false, "") | ||
|
|
||
| if !proceed { | ||
| return nil | ||
| return nil, nil | ||
| } | ||
| } | ||
|
|
||
| // App Manifest section | ||
| manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| configPath := filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename) | ||
|
|
@@ -170,26 +170,26 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty | |
| var auth *types.SlackAuth | ||
| *app, auth, err = promptExistingApp(ctx, clients) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| appIDs := []string{app.AppID} | ||
| _, err = clients.API().GetAppStatus(ctx, auth.Token, appIDs, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| // Save the app to the project | ||
| err = saveAppToJSON(ctx, clients, *app) | ||
| if err != nil { | ||
| clients.IO.PrintDebug(ctx, "Error saving app to file when linking existing app: %s", err) | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| // Footer section | ||
| LinkAppFooterSection(ctx, clients, app) | ||
|
|
||
| return nil | ||
| return auth, nil | ||
|
Member
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. 🔗 question: Instead of returning an 👾 ramble: I'm wanting to avoid mixing concepts with the |
||
| } | ||
|
|
||
| // LinkAppFooterSection displays the details of app that was added to the project. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ package project | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| import ( | ||||||||||||||||||||||
| "context" | ||||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||
| "math/rand" | ||||||||||||||||||||||
| "os" | ||||||||||||||||||||||
|
|
@@ -31,6 +32,7 @@ import ( | |||||||||||||||||||||
| "github.com/slackapi/slack-cli/internal/slackerror" | ||||||||||||||||||||||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||||||||||||||||||||||
| "github.com/slackapi/slack-cli/internal/style" | ||||||||||||||||||||||
| "github.com/spf13/afero" | ||||||||||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -229,8 +231,24 @@ func runCreateCommand(clients *shared.ClientFactory, cmd *cobra.Command, args [] | |||||||||||||||||||||
| defer func() { | ||||||||||||||||||||||
| _ = os.Chdir(originalDir) | ||||||||||||||||||||||
| }() | ||||||||||||||||||||||
| if err := app.LinkExistingApp(ctx, clients, &types.App{}, false); err != nil { | ||||||||||||||||||||||
| return err | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| linkedApp := &types.App{} | ||||||||||||||||||||||
| auth, linkErr := app.LinkExistingApp(ctx, clients, linkedApp, false) | ||||||||||||||||||||||
| if linkErr != nil { | ||||||||||||||||||||||
| return linkErr | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if auth != nil && linkedApp.AppID != "" { | ||||||||||||||||||||||
| fetchErr := fetchAndWriteRemoteManifest(ctx, clients, auth.Token, linkedApp.AppID, absProjectPath) | ||||||||||||||||||||||
| if fetchErr != nil { | ||||||||||||||||||||||
| 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", | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| })) | ||||||||||||||||||||||
|
Comment on lines
+244
to
+250
Member
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. |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -289,6 +307,21 @@ func printCreateSuccess(ctx context.Context, clients *shared.ClientFactory, appP | |||||||||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.CreateSuccess) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // 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, "", " ") | ||||||||||||||||||||||
|
Member
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: 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", |
||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||
| return err | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| data = append(data, '\n') | ||||||||||||||||||||||
| manifestPath := filepath.Join(projectPath, "manifest.json") | ||||||||||||||||||||||
| return afero.WriteFile(clients.Fs, manifestPath, data, 0644) | ||||||||||||||||||||||
|
Member
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. 🏁 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
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+310
to
+324
Member
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. 📠 suggestion: We might find a new file better for this? It might be shared in other commands or PRs #591! |
||||||||||||||||||||||
| // generateRandomAppName will create a random app name based on two words and a number | ||||||||||||||||||||||
| func generateRandomAppName() string { | ||||||||||||||||||||||
| rand.New(rand.NewSource(time.Now().UnixNano())) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.
suggestion: We should consider returning a single
prompts.SelectedApp(contains anappandauth) instead of mixing a pointer out-parameter (app *types.App) with the new returnedtypes.SlackAuth.The
appparameter 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,
appcomes out via the pointer whileauthcomes 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.SelectedAppalready pairsApp+Authand is the exact shape this function produces. Also,cmd/appalready importsinternal/prompts, so no new dependency.Suggested signature:
A zero
SelectedApp{}(i.e.selected.App.AppID == "") signals "user declined at the confirm prompt" - which is whatcreate.goalready 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.