Skip to content

Commit f5ab1ef

Browse files
mwbrooksmwbrooks
committed
feat: update remote manifest warnings for link command and manifest overwrite prompt
1 parent a87561b commit f5ab1ef

5 files changed

Lines changed: 36 additions & 145 deletions

File tree

‎cmd/app/link.go‎

Lines changed: 14 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ package app
1616

1717
import (
1818
"context"
19-
"fmt"
2019
"path/filepath"
2120
"strings"
2221

@@ -36,9 +35,6 @@ import (
3635
// LinkAppConfirmPromptText is displayed when prompting to add an existing app
3736
const LinkAppConfirmPromptText = "Do you want to add an existing app?"
3837

39-
// LinkAppManifestSourceConfirmPromptText is displayed before updating the manifest source
40-
const LinkAppManifestSourceConfirmPromptText = "Do you want to update the manifest source to remote?"
41-
4238
// appLinkFlagSet contains flag values to reference
4339
type appLinkFlagSet struct {
4440
environmentFlag string
@@ -131,11 +127,9 @@ func LinkAppHeaderSection(ctx context.Context, clients *shared.ClientFactory, sh
131127
}
132128

133129
// LinkExistingApp prompts for an existing App ID and saves the details to the project.
134-
// When shouldConfirm is true, a confirmation prompt will ask the user is they want to
130+
// When shouldConfirm is true, a confirmation prompt will ask the user if they want to
135131
// link an existing app and additional information is included in the header.
136132
// The shouldConfirm option is encouraged for third-party callers.
137-
// The link command requires manifest source to be remote. When it is not, a
138-
// confirmation prompt is displayed before updating the manifest source value.
139133
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (err error) {
140134
// Header section
141135
LinkAppHeaderSection(ctx, clients, shouldConfirm)
@@ -156,67 +150,20 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
156150
}
157151
}
158152

159-
// Confirm to update manifest source to remote.
160-
// - Update the manifest source to remote when its a GBP project with a local manifest.
161-
// - Do not update manifest source for ROSI projects, because they can only be local manifests.
162-
manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)
163-
isManifestSourceRemote := manifestSource.Equals(config.ManifestSourceRemote)
164-
isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil
165-
166-
if err != nil || (!isManifestSourceRemote && !isSlackHostedProject) {
167-
// When undefined, the default is config.ManifestSourceLocal
168-
if !manifestSource.Exists() {
169-
manifestSource = config.ManifestSourceLocal
170-
}
171-
172-
clients.IO.PrintInfo(ctx, false, "%s", style.Sectionf(style.TextSection{
173-
Emoji: "warning",
174-
Text: "Warning",
175-
Secondary: []string{
176-
"Linking an existing app requires the app manifest source to be managed by",
177-
fmt.Sprintf("%s.", config.ManifestSourceRemote.Human()),
178-
" ",
179-
fmt.Sprintf(`App manifest source can be %s or %s:`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
180-
fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
181-
fmt.Sprintf("- %s: uses manifest from app settings for each app", config.ManifestSourceRemote.String()),
182-
" ",
183-
fmt.Sprintf(style.Highlight(`Your manifest source is set to %s.`), manifestSource.Human()),
184-
" ",
185-
fmt.Sprintf("Current manifest source in %s:", style.Highlight(filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename))),
186-
fmt.Sprintf(style.Highlight(` %s: "%s"`), "manifest.source", manifestSource.String()),
187-
" ",
188-
fmt.Sprintf("Updating manifest source will be changed in %s:", style.Highlight(filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename))),
189-
fmt.Sprintf(style.Highlight(` %s: "%s"`), "manifest.source", config.ManifestSourceRemote),
190-
},
191-
}))
192-
193-
proceed, err := clients.IO.ConfirmPrompt(ctx, LinkAppManifestSourceConfirmPromptText, false)
194-
if err != nil {
195-
clients.IO.PrintDebug(ctx, "Error prompting to update the manifest source to %s: %s", config.ManifestSourceRemote, err)
196-
return err
197-
}
198-
199-
if !proceed {
200-
// Add newline to match the trailing newline inserted from the footer section
201-
clients.IO.PrintInfo(ctx, false, "")
202-
return nil
203-
}
204-
205-
if err := config.SetManifestSource(ctx, clients.Fs, clients.Os, config.ManifestSourceRemote); err != nil {
206-
// Log the error to the verbose output
207-
clients.IO.PrintDebug(ctx, "Error setting manifest source in project-level config: %s", err)
208-
// Display a user-friendly error with a workaround
209-
slackErr := slackerror.New(slackerror.ErrProjectConfigManifestSource).
210-
WithMessage("Failed to update the manifest source to %s", config.ManifestSourceRemote).
211-
WithRemediation(
212-
"You can manually update the manifest source by setting the following\nproperty in %s:\n %s",
213-
filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename),
214-
fmt.Sprintf(`manifest.source: "%s"`, config.ManifestSourceRemote),
215-
).
216-
WithRootCause(err)
217-
clients.IO.PrintError(ctx, "%s", slackErr.Error())
218-
}
153+
// App Manifest section
154+
manifestSource, _ := clients.Config.ProjectConfig.GetManifestSource(ctx)
155+
if !manifestSource.Exists() {
156+
manifestSource = config.ManifestSourceLocal
219157
}
158+
configPath := filepath.Join(config.ProjectConfigDirName, config.ProjectConfigJSONFilename)
159+
clients.IO.PrintInfo(ctx, false, "%s", style.Sectionf(style.TextSection{
160+
Emoji: "books",
161+
Text: "App Manifest",
162+
Secondary: []string{
163+
"Manifest source is " + style.Highlight(manifestSource.Human()),
164+
"Manifest source is configured in " + style.Highlight(configPath),
165+
},
166+
}))
220167

221168
// Prompt to get app details
222169
var auth *types.SlackAuth

‎cmd/app/link_test.go‎

Lines changed: 17 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -431,24 +431,18 @@ func Test_Apps_Link(t *testing.T) {
431431
CmdArgs: []string{},
432432
ExpectedError: slackerror.New(slackerror.ErrAppNotFound),
433433
},
434-
"accepting manifest source prompt should save information about the provided deployed app": {
434+
"links app when manifest source is local": {
435435
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
436436
cm.Auth.On("Auths", mock.Anything).Return([]types.SlackAuth{
437437
mockLinkSlackAuth2,
438438
mockLinkSlackAuth1,
439439
}, nil)
440440
cm.AddDefaultMocks()
441441
setupAppLinkCommandMocks(t, ctx, cm, cf)
442-
// Set manifest source to project to trigger confirmation prompt
442+
// Set manifest source to local
443443
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
444444
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
445445
}
446-
// Accept manifest source confirmation prompt
447-
cm.IO.On("ConfirmPrompt",
448-
mock.Anything,
449-
LinkAppManifestSourceConfirmPromptText,
450-
mock.Anything,
451-
).Return(true, nil)
452446
cm.IO.On("SelectPrompt",
453447
mock.Anything,
454448
"Select the existing app team",
@@ -496,49 +490,13 @@ func Test_Apps_Link(t *testing.T) {
496490
)
497491
require.NoError(t, err)
498492
assert.Equal(t, expectedApp, actualApp)
499-
// Assert manifest confirmation prompt accepted
500-
cm.IO.AssertCalled(t, "ConfirmPrompt",
501-
mock.Anything,
502-
LinkAppManifestSourceConfirmPromptText,
503-
mock.Anything,
504-
)
493+
// Assert manifest source info is displayed
494+
output := cm.GetCombinedOutput()
495+
assert.Contains(t, output, "App Manifest")
496+
assert.Contains(t, output, `"project" (local)`)
505497
},
506498
},
507-
"declining manifest source prompt should not link app": {
508-
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
509-
cm.AddDefaultMocks()
510-
setupAppLinkCommandMocks(t, ctx, cm, cf)
511-
// Set manifest source to project to trigger confirmation prompt
512-
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
513-
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
514-
}
515-
// Decline manifest source confirmation prompt
516-
cm.IO.On("ConfirmPrompt",
517-
mock.Anything,
518-
LinkAppManifestSourceConfirmPromptText,
519-
mock.Anything,
520-
).Return(false, nil)
521-
},
522-
CmdArgs: []string{},
523-
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
524-
// Assert manifest confirmation prompt accepted
525-
cm.IO.AssertCalled(t, "ConfirmPrompt",
526-
mock.Anything,
527-
LinkAppManifestSourceConfirmPromptText,
528-
mock.Anything,
529-
)
530-
531-
// Assert no apps saved
532-
apps, _, err := cm.AppClient.GetDeployedAll(ctx)
533-
require.NoError(t, err)
534-
require.Len(t, apps, 0)
535-
536-
apps, err = cm.AppClient.GetLocalAll(ctx)
537-
require.NoError(t, err)
538-
require.Len(t, apps, 0)
539-
},
540-
},
541-
"manifest source prompt should not display for Run-on-Slack apps with local manifest source": {
499+
"displays manifest info for Run-on-Slack apps with local manifest source": {
542500
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
543501
cm.Auth.On("Auths", mock.Anything).Return([]types.SlackAuth{
544502
mockLinkSlackAuth1,
@@ -607,15 +565,13 @@ func Test_Apps_Link(t *testing.T) {
607565
)
608566
require.NoError(t, err)
609567
assert.Equal(t, expectedApp, actualApp)
610-
// Assert manifest confirmation prompt was not displayed
611-
cm.IO.AssertNotCalled(t, "ConfirmPrompt",
612-
mock.Anything,
613-
LinkAppManifestSourceConfirmPromptText,
614-
mock.Anything,
615-
)
568+
// Assert manifest source info is displayed
569+
output := cm.GetCombinedOutput()
570+
assert.Contains(t, output, "App Manifest")
571+
assert.Contains(t, output, `"project" (local)`)
616572
},
617573
},
618-
"manifest source prompt should display for GBP apps with local manifest source": {
574+
"displays manifest info for GBP apps with local manifest source": {
619575
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
620576
cm.Auth.On("Auths", mock.Anything).Return([]types.SlackAuth{
621577
mockLinkSlackAuth1,
@@ -627,15 +583,10 @@ func Test_Apps_Link(t *testing.T) {
627583
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
628584
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
629585
}
630-
// Mock manifest for Run-on-Slack app
586+
// Mock manifest for GBP app
631587
manifestMock := &app.ManifestMockObject{}
632588
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil)
633589
cf.AppClient().Manifest = manifestMock
634-
cm.IO.On("ConfirmPrompt",
635-
mock.Anything,
636-
LinkAppManifestSourceConfirmPromptText,
637-
mock.Anything,
638-
).Return(true, nil)
639590
cm.IO.On("SelectPrompt",
640591
mock.Anything,
641592
"Select the existing app team",
@@ -683,12 +634,10 @@ func Test_Apps_Link(t *testing.T) {
683634
)
684635
require.NoError(t, err)
685636
assert.Equal(t, expectedApp, actualApp)
686-
// Assert manifest confirmation prompt was displayed
687-
cm.IO.AssertCalled(t, "ConfirmPrompt",
688-
mock.Anything,
689-
LinkAppManifestSourceConfirmPromptText,
690-
mock.Anything,
691-
)
637+
// Assert manifest source info is displayed
638+
output := cm.GetCombinedOutput()
639+
assert.Contains(t, output, "App Manifest")
640+
assert.Contains(t, output, `"project" (local)`)
692641
},
693642
},
694643
}, func(clients *shared.ClientFactory) *cobra.Command {

‎cmd/project/init_test.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,8 @@ func Test_Project_InitCommand(t *testing.T) {
108108
}, nil)
109109
// Default setup
110110
setupProjectInitCommandMocks(t, ctx, cm, cf)
111-
// Do not link an existing app
111+
// Link an existing app
112112
cm.IO.On("ConfirmPrompt", mock.Anything, app.LinkAppConfirmPromptText, mock.Anything).Return(true, nil)
113-
// Mock prompt to link an existing app
114-
cm.IO.On("ConfirmPrompt", mock.Anything, app.LinkAppManifestSourceConfirmPromptText, mock.Anything).Return(true, nil)
115113
// Mock prompt for team
116114
cm.IO.On("SelectPrompt",
117115
mock.Anything,

‎internal/pkg/apps/install.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,7 @@ func shouldUpdateManifest(ctx context.Context, clients *shared.ClientFactory, ap
752752
case saved.Equals(""):
753753
notice = "Manifest values for this app are overwritten on reinstall"
754754
default:
755-
notice = "The manifest on app settings has been changed since last update!"
755+
notice = style.Yellow("The manifest on app settings has been changed since last update")
756756
}
757757
clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{
758758
Emoji: "books",
@@ -764,10 +764,7 @@ func shouldUpdateManifest(ctx context.Context, clients *shared.ClientFactory, ap
764764
}
765765
continues, err := clients.IO.ConfirmPrompt(
766766
ctx,
767-
fmt.Sprintf(
768-
"Update app settings with changes to the %s manifest?",
769-
config.ManifestSourceLocal.String(),
770-
),
767+
"Overwrite manifest on app settings with the project's manifest file?",
771768
false,
772769
)
773770
if err != nil {

‎internal/pkg/apps/install_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,7 @@ func TestInstall(t *testing.T) {
574574
clientsMock.IO.On(
575575
"ConfirmPrompt",
576576
mock.Anything,
577-
"Update app settings with changes to the local manifest?",
577+
"Overwrite manifest on app settings with the project's manifest file?",
578578
false,
579579
).Return(
580580
tc.mockConfirmPrompt,
@@ -1396,7 +1396,7 @@ func TestInstallLocalApp(t *testing.T) {
13961396
clientsMock.IO.On(
13971397
"ConfirmPrompt",
13981398
mock.Anything,
1399-
"Update app settings with changes to the local manifest?",
1399+
"Overwrite manifest on app settings with the project's manifest file?",
14001400
false,
14011401
).Return(
14021402
tc.mockConfirmPrompt,

0 commit comments

Comments
 (0)