-
Notifications
You must be signed in to change notification settings - Fork 26
feat(bolt-install): support using 'install' command with remote manifests #154
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
Changes from 2 commits
7578b6e
46be447
7678bb4
9847d3c
10966f7
fda7732
aa42052
9b04aa7
521ee30
886568f
12d89f4
97fe3f0
edbcaf9
976d285
d62e7c6
013ad0d
1e2bd54
701a794
9e757b1
c868e1b
69e3284
0e65092
e28a02e
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 |
|---|---|---|
|
|
@@ -85,38 +85,36 @@ func preRunAddCommand(ctx context.Context, clients *shared.ClientFactory) error | |
| if !clients.Config.WithExperimentOn(experiment.BoltFrameworks) { | ||
| return nil | ||
| } | ||
| manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if manifestSource.Equals(config.ManifestSourceRemote) { | ||
| return slackerror.New(slackerror.ErrAppInstall). | ||
| WithMessage("Apps cannot be installed due to project configurations"). | ||
| WithRemediation( | ||
| "Install an app on app settings: %s\nLink an app to this project with %s\nList apps saved with this project using %s", | ||
| style.LinkText("https://api.slack.com/apps"), | ||
| style.Commandf("app link", false), | ||
| style.Commandf("app list", false), | ||
| ). | ||
| WithDetails(slackerror.ErrorDetails{ | ||
| slackerror.ErrorDetail{ | ||
| Code: slackerror.ErrProjectConfigManifestSource, | ||
| Message: "Cannot install apps with manifests sourced from app settings", | ||
| }, | ||
| }) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // RunAddCommand executes the workspace install command, prints output, and returns any errors. | ||
| func RunAddCommand(ctx context.Context, clients *shared.ClientFactory, selection *prompts.SelectedApp, orgGrantWorkspaceID string) (context.Context, types.InstallState, types.App, error) { | ||
| if selection == nil { | ||
| selected, err := teamAppSelectPromptFunc(ctx, clients, prompts.ShowHostedOnly, prompts.ShowAllApps) | ||
| // Prompt for deployed or local app environment. | ||
| isProductionApp, err := promptIsProduction(ctx, clients) | ||
| if err != nil { | ||
| return ctx, "", types.App{}, err | ||
| } | ||
|
|
||
| var appEnvironmentType prompts.AppEnvironmentType | ||
| if isProductionApp { | ||
| appEnvironmentType = prompts.ShowHostedOnly | ||
| } else { | ||
| appEnvironmentType = prompts.ShowLocalOnly | ||
| } | ||
|
|
||
| selected, err := teamAppSelectPromptFunc(ctx, clients, appEnvironmentType, prompts.ShowAllApps) | ||
|
||
| if err != nil { | ||
| return ctx, "", types.App{}, err | ||
| } | ||
| selection = &selected | ||
|
|
||
| if !isProductionApp { | ||
| selection.App.IsDev = true | ||
| } | ||
|
||
| } | ||
|
|
||
| if selection.Auth.TeamDomain == "" { | ||
| return ctx, "", types.App{}, slackerror.New(slackerror.ErrCredentialsNotFound) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,11 +24,11 @@ import ( | |
| "github.com/slackapi/slack-cli/internal/cmdutil" | ||
| "github.com/slackapi/slack-cli/internal/config" | ||
| "github.com/slackapi/slack-cli/internal/experiment" | ||
| "github.com/slackapi/slack-cli/internal/iostreams" | ||
| "github.com/slackapi/slack-cli/internal/prompts" | ||
| "github.com/slackapi/slack-cli/internal/shared" | ||
| "github.com/slackapi/slack-cli/internal/shared/types" | ||
| "github.com/slackapi/slack-cli/internal/slackerror" | ||
| "github.com/slackapi/slack-cli/internal/style" | ||
| "github.com/slackapi/slack-cli/test/testutil" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/mock" | ||
|
|
@@ -85,40 +85,27 @@ func TestAppAddCommandPreRun(t *testing.T) { | |
| cf.SDKConfig.WorkingDirectory = "." | ||
| }, | ||
| }, | ||
| "errors if manifest.source is remote with the bolt experiment": { | ||
| ExpectedError: slackerror.New(slackerror.ErrAppInstall). | ||
| WithMessage("Apps cannot be installed due to project configurations"). | ||
| WithRemediation( | ||
| "Install an app on app settings: %s\nLink an app to this project with %s\nList apps saved with this project using %s", | ||
| style.LinkText("https://api.slack.com/apps"), | ||
| style.Commandf("app link", false), | ||
| style.Commandf("app list", false), | ||
| ). | ||
| WithDetails(slackerror.ErrorDetails{ | ||
| slackerror.ErrorDetail{ | ||
| Code: slackerror.ErrProjectConfigManifestSource, | ||
| Message: "Cannot install apps with manifests sourced from app settings", | ||
| }, | ||
| }), | ||
|
Comment on lines
-88
to
-102
Member
Author
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. 🪓 We no longer want to error on a remote manifest, so this test is removed and replaced with
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. 🪓 thought: Once the
Member
Author
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. Yes please! |
||
| "proceeds if manifest.source is local with the bolt experiment": { | ||
| ExpectedError: nil, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| cm.AddDefaultMocks() | ||
| cm.Config.ExperimentsFlag = append(cm.Config.ExperimentsFlag, string(experiment.BoltFrameworks)) | ||
| cm.Config.LoadExperiments(ctx, cm.IO.PrintDebug) | ||
| mockProjectConfig := config.NewProjectConfigMock() | ||
| mockProjectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceRemote, nil) | ||
| mockProjectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) | ||
| cm.Config.ProjectConfig = mockProjectConfig | ||
| }, | ||
| }, | ||
| "proceeds if manifest.source is local with the bolt experiment": { | ||
| "proceeds if manifest.source is remote with the bolt experiment": { | ||
| ExpectedError: nil, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig.WorkingDirectory = "." | ||
| cm.AddDefaultMocks() | ||
| cm.Config.ExperimentsFlag = append(cm.Config.ExperimentsFlag, string(experiment.BoltFrameworks)) | ||
| cm.Config.LoadExperiments(ctx, cm.IO.PrintDebug) | ||
| mockProjectConfig := config.NewProjectConfigMock() | ||
| mockProjectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) | ||
| mockProjectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceRemote, nil) | ||
| cm.Config.ProjectConfig = mockProjectConfig | ||
| }, | ||
| }, | ||
|
|
@@ -130,13 +117,84 @@ func TestAppAddCommandPreRun(t *testing.T) { | |
| } | ||
|
|
||
| func TestAppAddCommand(t *testing.T) { | ||
|
|
||
| testutil.TableTestCommand(t, testutil.CommandTests{ | ||
| "adds a new local app": { | ||
|
Member
Author
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: Added test for new local apps to complement the existing |
||
| CmdArgs: []string{}, | ||
| ExpectedOutputs: []string{"Creating app manifest", "Installing"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm, "local") | ||
|
|
||
| // Mock TeamSelector prompt to return "team1" | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt | ||
| appSelectMock.On("TeamAppSelectPrompt").Return(prompts.SelectedApp{Auth: mockAuthTeam1}, nil) | ||
|
|
||
| // Mock valid session for team1 | ||
| cm.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{ | ||
| UserID: &mockAuthTeam1.UserID, | ||
| TeamID: &mockAuthTeam1.TeamID, | ||
| TeamName: &mockAuthTeam1.TeamDomain, | ||
| }, nil) | ||
|
|
||
| // Mock a clean ValidateAppManifest result | ||
| cm.API.On("ValidateAppManifest", mock.Anything, mockAuthTeam1.Token, mock.Anything, mock.Anything).Return( | ||
| api.ValidateAppManifestResult{ | ||
| Warnings: slackerror.Warnings{}, | ||
| }, nil, | ||
| ) | ||
|
|
||
| // Mock Host | ||
| cm.API.On("Host").Return("") | ||
|
|
||
| // Mock a successful CreateApp call and return our mocked AppID | ||
| cm.API.On("CreateApp", mock.Anything, mockAuthTeam1.Token, mock.Anything, mock.Anything).Return( | ||
| api.CreateAppResult{ | ||
| AppID: mockAppTeam1.AppID, | ||
| }, | ||
| nil, | ||
| ) | ||
|
|
||
| // Mock a successful DeveloperAppInstall | ||
| cm.API.On("DeveloperAppInstall", mock.Anything, mock.Anything, mockAuthTeam1.Token, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return( | ||
| api.DeveloperAppInstallResult{ | ||
| AppID: mockAppTeam1.AppID, | ||
| APIAccessTokens: struct { | ||
| Bot string "json:\"bot,omitempty\"" | ||
| AppLevel string "json:\"app_level,omitempty\"" | ||
| User string "json:\"user,omitempty\"" | ||
| }{}, | ||
| }, | ||
| types.InstallSuccess, | ||
| nil, | ||
| ) | ||
|
|
||
| // Mock existing and updated cache | ||
| cm.API.On( | ||
| "ExportAppManifest", | ||
| mock.Anything, | ||
| mock.Anything, | ||
| mock.Anything, | ||
| ).Return( | ||
| api.ExportAppResult{}, | ||
| nil, | ||
| ) | ||
| mockProjectCache := cache.NewCacheMock() | ||
| mockProjectCache.On("GetManifestHash", mock.Anything, mock.Anything). | ||
| Return(cache.Hash(""), nil) | ||
| mockProjectCache.On("NewManifestHash", mock.Anything, mock.Anything). | ||
| Return(cache.Hash("xoxo"), nil) | ||
| mockProjectCache.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything). | ||
| Return(nil) | ||
| mockProjectConfig := config.NewProjectConfigMock() | ||
| mockProjectConfig.On("Cache").Return(mockProjectCache) | ||
| cm.Config.ProjectConfig = mockProjectConfig | ||
| }, | ||
| }, | ||
| "adds a new deployed app": { | ||
| CmdArgs: []string{}, | ||
| ExpectedOutputs: []string{"Creating app manifest", "Installing"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
|
|
||
| // Mock TeamSelector prompt to return "team1" | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
|
|
@@ -208,7 +266,7 @@ func TestAppAddCommand(t *testing.T) { | |
| CmdArgs: []string{}, | ||
| ExpectedOutputs: []string{"Updated app manifest"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
|
|
||
| // Mock TeamSelector prompt to return "team1" | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
|
|
@@ -290,7 +348,7 @@ func TestAppAddCommand(t *testing.T) { | |
| CmdArgs: []string{}, | ||
| ExpectedError: slackerror.New(slackerror.ErrCredentialsNotFound), | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt | ||
| appSelectMock.On("TeamAppSelectPrompt").Return(prompts.SelectedApp{App: mockAppTeam1}, nil) | ||
|
|
@@ -300,7 +358,7 @@ func TestAppAddCommand(t *testing.T) { | |
| CmdArgs: []string{"--" + cmdutil.OrgGrantWorkspaceFlag, "T123"}, | ||
| ExpectedOutputs: []string{"Creating app manifest", "Installing"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
| // Select workspace | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt | ||
|
|
@@ -360,7 +418,7 @@ func TestAppAddCommand(t *testing.T) { | |
| CmdArgs: []string{"--" + cmdutil.OrgGrantWorkspaceFlag, "T123"}, | ||
| ExpectedOutputs: []string{"Creating app manifest", "Installing", "Your request to install the app is pending", "complete installation by re-running"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
| // Select workspace | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt | ||
|
|
@@ -416,7 +474,7 @@ func TestAppAddCommand(t *testing.T) { | |
| CmdArgs: []string{"--" + cmdutil.OrgGrantWorkspaceFlag, "T123"}, | ||
| ExpectedOutputs: []string{"Creating app manifest", "Installing", "Your request to install the app has been cancelled"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| prepareAddMocks(t, cf, cm) | ||
| prepareAddMocks(t, cf, cm, "deployed") | ||
| // Select workspace | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt | ||
|
|
@@ -475,7 +533,7 @@ func TestAppAddCommand(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| func prepareAddMocks(t *testing.T, clients *shared.ClientFactory, clientsMock *shared.ClientsMock) { | ||
| func prepareAddMocks(t *testing.T, clients *shared.ClientFactory, clientsMock *shared.ClientsMock, appEnvironment string) { | ||
| clientsMock.AddDefaultMocks() | ||
|
|
||
| clientsMock.Auth.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything). | ||
|
|
@@ -498,4 +556,16 @@ func prepareAddMocks(t *testing.T, clients *shared.ClientFactory, clientsMock *s | |
| listPkgMock := new(ListPkgMock) | ||
| listFunc = listPkgMock.List | ||
| listPkgMock.On("List").Return(nil) | ||
|
|
||
| // Mock the prompt to select the app environment. | ||
| clientsMock.IO.On("SelectPrompt", | ||
| mock.Anything, | ||
| "Choose the app environment", | ||
| mock.Anything, | ||
| mock.Anything, | ||
| mock.Anything, | ||
| ).Return(iostreams.SelectPromptResponse{ | ||
| Flag: true, | ||
| Option: appEnvironment, | ||
| }, nil) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,16 +163,24 @@ func hasValidDeploymentMethod( | |
| return err | ||
| } | ||
| switch { | ||
| // When the manifest source is local, we can get the manifest from the local project. | ||
| case manifestSource.Equals(config.ManifestSourceLocal): | ||
| manifest, err = clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| case manifestSource.Equals(config.ManifestSourceRemote): | ||
| // When the manifest source is remote and the app exists, we can get the manifest from the the API. | ||
| case manifestSource.Equals(config.ManifestSourceRemote) && app.AppID != "": | ||
|
Comment on lines
+171
to
+172
Member
Author
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: We need to differentiate between a remote manifest with an app (get manifest from app settings) and a remote manifest without an app (get manifest from local file).
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. 👁️🗨️ thought: An |
||
| manifest, err = clients.AppClient().Manifest.GetManifestRemote(ctx, auth.Token, app.AppID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // When the app does not exist, we need to get the manifest from the local project. | ||
| default: | ||
| manifest, err = clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if manifest.FunctionRuntime() == types.SlackHosted { | ||
| return nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,6 +111,7 @@ func TestDeployCommand(t *testing.T) { | |
|
|
||
| func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) { | ||
| tests := map[string]struct { | ||
| app types.App | ||
| manifest types.SlackYaml | ||
| manifestError error | ||
| manifestSource config.ManifestSource | ||
|
|
@@ -144,6 +145,20 @@ func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) { | |
| manifestSource: config.ManifestSourceLocal, | ||
| expectedError: slackerror.New(slackerror.ErrSDKHookNotFound), | ||
| }, | ||
| "succeeds if the app exists and the manifest source is remote": { | ||
|
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. 😉 thought: These are clever tests. I think it calls out the importance of error codes in assertions. But seems right here! |
||
| app: types.App{ | ||
| AppID: "A123", | ||
| }, | ||
| manifestSource: config.ManifestSourceRemote, | ||
| expectedError: slackerror.New(slackerror.ErrSDKHookNotFound), | ||
| }, | ||
| "succeeds if the app does not exist and the manifest source is remote": { | ||
| app: types.App{ | ||
| AppID: "", | ||
| }, | ||
| manifestSource: config.ManifestSourceRemote, | ||
| expectedError: slackerror.New(slackerror.ErrSDKHookNotFound), | ||
| }, | ||
| } | ||
| for name, tt := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
|
|
@@ -152,6 +167,7 @@ func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) { | |
| clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) { | ||
| manifestMock := &app.ManifestMockObject{} | ||
| manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(tt.manifest, tt.manifestError) | ||
| manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything).Return(tt.manifest, tt.manifestError) | ||
| clients.AppClient().Manifest = manifestMock | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On("GetManifestSource", mock.Anything). | ||
|
|
@@ -160,7 +176,11 @@ func TestDeployCommand_HasValidDeploymentMethod(t *testing.T) { | |
| clients.SDKConfig = hooks.NewSDKConfigMock() | ||
| clients.SDKConfig.Hooks.Deploy.Command = tt.deployScript | ||
| }) | ||
| err := hasValidDeploymentMethod(ctx, clients, types.App{}, types.SlackAuth{}) | ||
| app := types.App{} | ||
| if tt.app.AppID != "" { | ||
| app = tt.app | ||
| } | ||
| err := hasValidDeploymentMethod(ctx, clients, app, types.SlackAuth{}) | ||
| if tt.expectedError != nil { | ||
| require.Error(t, err) | ||
| assert.Equal( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,13 +8,14 @@ The following is a list of currently available experiments. We'll remove experim | |||||
|
|
||||||
| * `bolt-install`: enables creating, installing, and running Bolt projects that manage their app manifest on app settings (remote manifest). | ||||||
| * `slack create` and `slack init` now set manifest source to "app settings" (remote) for Bolt JS & Bolt Python projects ([PR#96](https://github.com/slackapi/slack-cli/pull/96)). | ||||||
| * `slack run` and `slack install` support creating and installing Bolt Framework apps that have the manifest source set to "app settings (remote)" ([PR#111](https://github.com/slackapi/slack-cli/pull/111), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). | ||||||
|
||||||
| * `slack run` and `slack install` support creating and installing Bolt Framework apps that have the manifest source set to "app settings (remote)" ([PR#111](https://github.com/slackapi/slack-cli/pull/111), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). | |
| * `slack run` and `slack install` support creating and installing Bolt Framework apps that have the manifest source set to "app settings (remote)" ([PR#111](https://github.com/slackapi/slack-cli/pull/111), [PR#TODO](https://github.com/slackapi/slack-cli/pull/TODO)). |
assuming this #TODO is still needed to be filled out?
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.
Whew! Thank you, @lukegalbraithrussell! Commit 10966f7 adds the PR number.
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.
🪓 Removing this section that prevents the
slack installcommand from executing on a remote manifest.