From 471a8480c0899b7f3f1be191c16b1abaaae941d6 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Mon, 21 Apr 2025 16:01:59 -0700 Subject: [PATCH 1/4] feat: update 'app link' to support run-on-slack apps with local manifest src --- cmd/app/link.go | 8 ++- cmd/app/link_test.go | 167 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 168 insertions(+), 7 deletions(-) diff --git a/cmd/app/link.go b/cmd/app/link.go index bdbd57ea..86eadc1a 100644 --- a/cmd/app/link.go +++ b/cmd/app/link.go @@ -115,7 +115,7 @@ func LinkCommandRunE(ctx context.Context, clients *shared.ClientFactory, app *ty // explaining how to link apps, in case the user declines. func LinkAppHeaderSection(ctx context.Context, clients *shared.ClientFactory, shouldConfirm bool) { var secondaryText = []string{ - "Add an existing app created on app settings", + "Add an existing app from app settings", "Find your existing apps at: " + style.Underline("https://api.slack.com/apps"), } @@ -156,10 +156,12 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty } } - // Confirm to update manifest source to remote + // Confirm to update manifest source to remote. + // - Update the manifest source to remote when its a GBP project with a local manifest. + // - Do not update manifest source for ROSI projects, because they can only be local manifests. manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) - if !manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) || err != nil { + if err != nil || (!manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) && cmdutil.IsSlackHostedProject(ctx, clients) != nil) { // When undefined, the default is config.MANIFEST_SOURCE_LOCAL if !manifestSource.Exists() { manifestSource = config.MANIFEST_SOURCE_LOCAL diff --git a/cmd/app/link_test.go b/cmd/app/link_test.go index 511b54f7..4d535cb0 100644 --- a/cmd/app/link_test.go +++ b/cmd/app/link_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/slackapi/slack-cli/internal/api" + "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/iostreams" "github.com/slackapi/slack-cli/internal/shared" @@ -430,7 +431,7 @@ func Test_Apps_Link(t *testing.T) { CmdArgs: []string{}, ExpectedError: slackerror.New(slackerror.ErrAppNotFound), }, - "accept manifest source prompt and saves information about the provided deployed app": { + "accepting manifest source prompt should save information about the provided deployed app": { Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{ mockLinkSlackAuth2, @@ -503,7 +504,7 @@ func Test_Apps_Link(t *testing.T) { ) }, }, - "decline manifest source prompt should not link app": { + "declining manifest source prompt should not link app": { Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { cm.AddDefaultMocks() setupAppLinkCommandMocks(t, ctx, cm, cf) @@ -537,6 +538,159 @@ func Test_Apps_Link(t *testing.T) { require.Len(t, apps, 0) }, }, + "manifest source prompt should not display for Run-on-Slack apps with local manifest source": { + Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { + cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{ + mockLinkSlackAuth1, + mockLinkSlackAuth2, + }, nil) + cm.AddDefaultMocks() + setupAppLinkCommandMocks(t, ctx, cm, cf) + // Set manifest source to local + if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil { + require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err)) + } + // Mock manifest for Run-on-Slack app + manifestMock := &app.ManifestMockObject{} + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{ + AppManifest: types.AppManifest{ + Settings: &types.AppSettings{ + FunctionRuntime: types.SLACK_HOSTED, + }, + }, + }, nil) + cf.AppClient().Manifest = manifestMock + cm.IO.On("SelectPrompt", + mock.Anything, + "Select the existing app team", + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(iostreams.SelectPromptResponse{ + Flag: true, + Option: mockLinkSlackAuth1.TeamDomain, + }, nil) + cm.IO.On("InputPrompt", + mock.Anything, + "Enter the existing app ID", + mock.Anything, + ).Return(mockLinkAppID1, nil) + cm.IO.On("SelectPrompt", + mock.Anything, + "Choose the app environment", + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(iostreams.SelectPromptResponse{ + Flag: true, + Option: "deployed", + }, nil) + cm.ApiInterface.On( + "GetAppStatus", + mock.Anything, + mockLinkSlackAuth1.Token, + []string{mockLinkAppID1}, + mockLinkSlackAuth1.TeamID, + ).Return(api.GetAppStatusResult{}, nil) + }, + CmdArgs: []string{}, + ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { + expectedApp := types.App{ + AppID: mockLinkAppID1, + TeamDomain: mockLinkSlackAuth1.TeamDomain, + TeamID: mockLinkSlackAuth1.TeamID, + EnterpriseID: mockLinkSlackAuth1.EnterpriseID, + } + actualApp, err := cm.AppClient.GetDeployed( + ctx, + mockLinkSlackAuth1.TeamID, + ) + require.NoError(t, err) + assert.Equal(t, expectedApp, actualApp) + // Assert manifest confirmation prompt was not displayed + cm.IO.AssertNotCalled(t, "ConfirmPrompt", + mock.Anything, + LinkAppManifestSourceConfirmPromptText, + mock.Anything, + ) + }, + }, + "manifest source prompt should display for GBP apps with local manifest source": { + Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { + cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{ + mockLinkSlackAuth1, + mockLinkSlackAuth2, + }, nil) + cm.AddDefaultMocks() + setupAppLinkCommandMocks(t, ctx, cm, cf) + // Set manifest source to local + if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil { + require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err)) + } + // Mock manifest for Run-on-Slack app + manifestMock := &app.ManifestMockObject{} + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + cf.AppClient().Manifest = manifestMock + cm.IO.On("ConfirmPrompt", + mock.Anything, + LinkAppManifestSourceConfirmPromptText, + mock.Anything, + ).Return(true, nil) + cm.IO.On("SelectPrompt", + mock.Anything, + "Select the existing app team", + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(iostreams.SelectPromptResponse{ + Flag: true, + Option: mockLinkSlackAuth1.TeamDomain, + }, nil) + cm.IO.On("InputPrompt", + mock.Anything, + "Enter the existing app ID", + mock.Anything, + ).Return(mockLinkAppID1, nil) + cm.IO.On("SelectPrompt", + mock.Anything, + "Choose the app environment", + mock.Anything, + mock.Anything, + mock.Anything, + ).Return(iostreams.SelectPromptResponse{ + Flag: true, + Option: "deployed", + }, nil) + cm.ApiInterface.On( + "GetAppStatus", + mock.Anything, + mockLinkSlackAuth1.Token, + []string{mockLinkAppID1}, + mockLinkSlackAuth1.TeamID, + ).Return(api.GetAppStatusResult{}, nil) + }, + CmdArgs: []string{}, + ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { + expectedApp := types.App{ + AppID: mockLinkAppID1, + TeamDomain: mockLinkSlackAuth1.TeamDomain, + TeamID: mockLinkSlackAuth1.TeamID, + EnterpriseID: mockLinkSlackAuth1.EnterpriseID, + } + actualApp, err := cm.AppClient.GetDeployed( + ctx, + mockLinkSlackAuth1.TeamID, + ) + require.NoError(t, err) + assert.Equal(t, expectedApp, actualApp) + // Assert manifest confirmation prompt was displayed + cm.IO.AssertCalled(t, "ConfirmPrompt", + mock.Anything, + LinkAppManifestSourceConfirmPromptText, + mock.Anything, + ) + }, + }, }, func(clients *shared.ClientFactory) *cobra.Command { clients.SDKConfig.WorkingDirectory = "." return NewLinkCommand(clients) @@ -552,7 +706,7 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) { "When shouldConfirm is false": { shouldConfirm: false, expectedOutputs: []string{ - "Add an existing app created on app settings", + "Add an existing app from app settings", "Find your existing apps at: https://api.slack.com/apps", }, unexpectedOutputs: []string{ @@ -562,7 +716,7 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) { "When shouldConfirm is true": { shouldConfirm: true, expectedOutputs: []string{ - "Add an existing app created on app settings", + "Add an existing app from app settings", "Find your existing apps at: https://api.slack.com/apps", "Manually add apps later with", }, @@ -609,4 +763,9 @@ func setupAppLinkCommandMocks(t *testing.T, ctx context.Context, cm *shared.Clie if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_REMOTE); err != nil { require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err)) } + + // Mock manifest + manifestMock := &app.ManifestMockObject{} + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + cf.AppClient().Manifest = manifestMock } From 9a5be4c296e27c834292175ac227e78a9cfc64de Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Mon, 21 Apr 2025 16:47:25 -0700 Subject: [PATCH 2/4] test: fix failing 'project init' test --- cmd/project/init_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cmd/project/init_test.go b/cmd/project/init_test.go index 576da4e3..5979d3bd 100644 --- a/cmd/project/init_test.go +++ b/cmd/project/init_test.go @@ -22,6 +22,7 @@ import ( "github.com/slackapi/slack-cli/cmd/app" "github.com/slackapi/slack-cli/internal/api" + internalApp "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/iostreams" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" @@ -237,4 +238,9 @@ func setupProjectInitCommandMocks(t *testing.T, ctx context.Context, cm *shared. if err := cm.Fs.MkdirAll(filepath.Dir(projectDirPath), 0755); err != nil { require.FailNow(t, fmt.Sprintf("Failed to create the directory %s in the memory-based file system", projectDirPath)) } + + // Mock manifest + manifestMock := &internalApp.ManifestMockObject{} + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + cf.AppClient().Manifest = manifestMock } From ce180eb5a65b2170421e939205636486a7303c80 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Mon, 21 Apr 2025 17:00:18 -0700 Subject: [PATCH 3/4] test: update tests with latest ctx changes --- cmd/app/link_test.go | 6 +++--- cmd/project/init_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/app/link_test.go b/cmd/app/link_test.go index 4d535cb0..1e125f06 100644 --- a/cmd/app/link_test.go +++ b/cmd/app/link_test.go @@ -552,7 +552,7 @@ func Test_Apps_Link(t *testing.T) { } // Mock manifest for Run-on-Slack app manifestMock := &app.ManifestMockObject{} - manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{ + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{ AppManifest: types.AppManifest{ Settings: &types.AppSettings{ FunctionRuntime: types.SLACK_HOSTED, @@ -629,7 +629,7 @@ func Test_Apps_Link(t *testing.T) { } // Mock manifest for Run-on-Slack app manifestMock := &app.ManifestMockObject{} - manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) cf.AppClient().Manifest = manifestMock cm.IO.On("ConfirmPrompt", mock.Anything, @@ -766,6 +766,6 @@ func setupAppLinkCommandMocks(t *testing.T, ctx context.Context, cm *shared.Clie // Mock manifest manifestMock := &app.ManifestMockObject{} - manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) cf.AppClient().Manifest = manifestMock } diff --git a/cmd/project/init_test.go b/cmd/project/init_test.go index 5979d3bd..4a8f87b2 100644 --- a/cmd/project/init_test.go +++ b/cmd/project/init_test.go @@ -241,6 +241,6 @@ func setupProjectInitCommandMocks(t *testing.T, ctx context.Context, cm *shared. // Mock manifest manifestMock := &internalApp.ManifestMockObject{} - manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) + manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil) cf.AppClient().Manifest = manifestMock } From 30e69a79f90cf43bd913cc046054ce22a443f91b Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Tue, 22 Apr 2025 10:24:59 -0700 Subject: [PATCH 4/4] refactor: simplify the manifest update conditional --- cmd/app/link.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/app/link.go b/cmd/app/link.go index 86eadc1a..5543319a 100644 --- a/cmd/app/link.go +++ b/cmd/app/link.go @@ -160,8 +160,10 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty // - Update the manifest source to remote when its a GBP project with a local manifest. // - Do not update manifest source for ROSI projects, because they can only be local manifests. manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) + isManifestSourceRemote := manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) + isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil - if err != nil || (!manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) && cmdutil.IsSlackHostedProject(ctx, clients) != nil) { + if err != nil || (!isManifestSourceRemote && !isSlackHostedProject) { // When undefined, the default is config.MANIFEST_SOURCE_LOCAL if !manifestSource.Exists() { manifestSource = config.MANIFEST_SOURCE_LOCAL