feat(create): cleanup app creation outputs when slack create -app passed#598
feat(create): cleanup app creation outputs when slack create -app passed#598srtaalej wants to merge 6 commits into
slack create -app passed#598Conversation
srtaalej
added
enhancement
srtaalej
changed the title
slack create -app passed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #598 +/- ##
==========================================
- Coverage 71.68% 71.68% -0.01%
==========================================
Files 226 226
Lines 19176 19183 +7
==========================================
+ Hits 13747 13751 +4
+ Misses 4220 4218 -2
- Partials 1209 1214 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // LinkExistingAppQuiet links an existing app without verbose output. It resolves | ||
| // the team and environment, validates the app, saves it, and prints only the app | ||
| // summary. Used by `create --app` where the surrounding create output is enough. | ||
| func LinkExistingAppQuiet(ctx context.Context, clients *shared.ClientFactory, app *types.App) error { | ||
| linkedApp, auth, err := promptExistingApp(ctx, clients) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| *app = linkedApp | ||
|
|
||
| appIDs := []string{app.AppID} | ||
| _, err = clients.API().GetAppStatus(ctx, auth.Token, appIDs, app.TeamID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||
| Emoji: "house", | ||
| Text: "App", | ||
| Secondary: formatListSuccess([]types.App{*app}), | ||
| })) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🔭 thought: This is near identical to the lines above and I'm wondering if we can somehow keep a single source of truth for this logic?
Lines 170 to 190 in a0f86d7
There was a problem hiding this comment.
sure good call! @zimeg to clarify though, do you mean the entire link command should return the way LinkExistingAppQuiet is returning? or should we have something like
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool, quiet bool) (err error) {
if !quiet {
// Header section
LinkAppHeaderSection(ctx, clients, shouldConfirm)
}
There was a problem hiding this comment.
@srtaalej I missed the edited mention! question But separating output from logic might make this possible? I think outputs and even the "should confirm" logic should be moved to adjacent functions:
- app.LinkExistingApp: What I permalink above
- app.LinkCommandRunE: Perhaps most the "link" outputs not meant to share
|
|
||
| // Add an existing app to the project | ||
| err = app.LinkExistingApp(ctx, clients, &types.App{}, true) | ||
| err = app.LinkCommandRunE(ctx, clients, &types.App{}, true, false) |
There was a problem hiding this comment.
💡 thought: No blocker but we might want to refactor the confirmation prompt to be here instead of within the link command?
⚡ ramble: I find boolean arguments unclear and also think a command should run if it's called - it'd be up to the caller to decide if that command should run instead!
| } | ||
|
|
||
| func Test_Apps_LinkAppHeaderSection(t *testing.T) { | ||
| tests := map[string]struct { |
There was a problem hiding this comment.
🏗️ quibble: Keeping table tests structured - even if one case - instead of a flattened single case might encourage additional cases
| "When shouldConfirm is true": { | ||
| shouldConfirm: true, | ||
| expectedOutputs: []string{ | ||
| "Add an existing app from app settings", | ||
| "Find your existing apps at: https://api.slack.com/apps", | ||
| "Manually add apps later with", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔬 note: Existing tests continue to cover this removed case in the init command.
slack-cli/cmd/project/init_test.go
Lines 56 to 207 in 4565149
| clients.IO.PrintInfo(ctx, false, "%s", style.Sectionf(style.TextSection{ | ||
| Emoji: "house", | ||
| Text: "App", | ||
| Secondary: app.FormatListSuccess([]types.App{*linkedApp}), | ||
| })) |
There was a problem hiding this comment.
🌟 praise: Inlining this output within create seems to've fixed an extra linebreak!
| _ = os.Chdir(originalDir) | ||
| }() | ||
| if err := app.LinkExistingApp(ctx, clients, &types.App{}, false); err != nil { | ||
| linkedApp := &types.App{} |
There was a problem hiding this comment.
🔭 note: IIRC adjacent PR has discussion about returning instead of providing an app argument. No blocker but it has support in eventual follow up!
| // LinkAppConfirmPromptText is displayed when prompting to add an existing app | ||
| const LinkAppConfirmPromptText = "Do you want to add an existing app?" |
There was a problem hiding this comment.
👾 suggestion: This is now used for init and not within link so I suggest we move it to the "project" package and make it private?
| if err != nil { | ||
| // Display the error but continue to init | ||
| clients.IO.PrintError(ctx, "%s", err.Error()) | ||
| clients.IO.PrintDebug(ctx, "Error prompting to add an existing app: %s", err) |
There was a problem hiding this comment.
| clients.IO.PrintDebug(ctx, "Error prompting to add an existing app: %s", err) | |
| return err |
🗣️ suggestion: The earlier debug might've been too generic? Since we've moved this prompt callsite to init we might want to return this prompt error to stop the command after interrupts?
| err = app.LinkExistingApp(ctx, clients, &types.App{}, true) | ||
| // Prompt to add an existing app to the project | ||
| app.LinkAppHeaderSection(ctx, clients) | ||
| clients.IO.PrintInfo(ctx, false, " %s\n", "Manually add apps later with "+style.Commandf("app link", true)) |
There was a problem hiding this comment.
| clients.IO.PrintInfo(ctx, false, " %s\n", "Manually add apps later with "+style.Commandf("app link", true)) |
🪓 suggestion: Let's remove this to avoid duplication in later output
| // Prompt to add an existing app to the project | ||
| app.LinkAppHeaderSection(ctx, clients) | ||
| clients.IO.PrintInfo(ctx, false, " %s\n", "Manually add apps later with "+style.Commandf("app link", true)) | ||
| proceed, err := clients.IO.ConfirmPrompt(ctx, app.LinkAppConfirmPromptText, true) |
There was a problem hiding this comment.
🌚 praise: Bringing this confirmation prompt to the command that uses it is great! It unlocks more meaningful checks IMHO like checking if an app is saved before prompting.
3 participants
Changelog
Reduced verbose output when using
create --appto link an existing app during project creation.Summary
When
--appis provided toslack create, the linking step previously printed multiple noisy sections (App Link header, App Manifest details, App footer, and "Added existing app" message). Now it only prints the single🏠 Appsummary section showing team, App ID, Team ID, User ID, and Status.Preview
Before
After
Testing
make test./bin/slack create my-test -t slack-samples/bolt-js-starter-template --app A123 --team T123 --environment localNotes
Adresses comments on PR #565 - Improve the output experience for create when --app is provided.
Requirements