-
Notifications
You must be signed in to change notification settings - Fork 32
feat: env add command supports the ".env" file for Bolt Framework apps
#451
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
986b209
9f90561
81b16ac
b85d49a
ed80e34
ae490ab
bac8afa
98a23ea
774e2b1
22a7f23
51641d2
9d19722
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ import ( | |||||||||||||
| "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/slackdotenv" | ||||||||||||||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||||||||||||||
| "github.com/slackapi/slack-cli/internal/style" | ||||||||||||||
| "github.com/spf13/cobra" | ||||||||||||||
|
|
@@ -31,14 +32,17 @@ import ( | |||||||||||||
| func NewEnvAddCommand(clients *shared.ClientFactory) *cobra.Command { | ||||||||||||||
| cmd := &cobra.Command{ | ||||||||||||||
| Use: "add <name> <value> [flags]", | ||||||||||||||
| Short: "Add an environment variable to the app", | ||||||||||||||
| Short: "Add an environment variable to the project", | ||||||||||||||
| Long: strings.Join([]string{ | ||||||||||||||
| "Add an environment variable to an app deployed to Slack managed infrastructure.", | ||||||||||||||
| "Add an environment variable to the project.", | ||||||||||||||
| "", | ||||||||||||||
| "If a name or value is not provided, you will be prompted to provide these.", | ||||||||||||||
| "", | ||||||||||||||
| "This command is supported for apps deployed to Slack managed infrastructure but", | ||||||||||||||
| "other apps can attempt to run the command with the --force flag.", | ||||||||||||||
| "Commands that run in the context of a project source environment variables from", | ||||||||||||||
| "the \".env\" file. This includes the \"run\" command.", | ||||||||||||||
| "", | ||||||||||||||
| "The \"deploy\" command gathers environment variables from the \".env\" file as well", | ||||||||||||||
|
Comment on lines
+42
to
+44
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(non-blocking): Please ignore if you want, but thought this may be a chance to keep the code simpler.
Suggested change
|
||||||||||||||
| "unless the app is using ROSI features.", | ||||||||||||||
| }, "\n"), | ||||||||||||||
| Example: style.ExampleCommandsf([]style.ExampleCommand{ | ||||||||||||||
| { | ||||||||||||||
|
|
@@ -69,26 +73,19 @@ func NewEnvAddCommand(clients *shared.ClientFactory) *cobra.Command { | |||||||||||||
| return cmd | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // preRunEnvAddCommandFunc determines if the command is supported for a project | ||||||||||||||
| // preRunEnvAddCommandFunc determines if the command is run in a valid project | ||||||||||||||
| // and configures flags | ||||||||||||||
| func preRunEnvAddCommandFunc(ctx context.Context, clients *shared.ClientFactory, cmd *cobra.Command) error { | ||||||||||||||
| clients.Config.SetFlags(cmd) | ||||||||||||||
| err := cmdutil.IsValidProjectDirectory(clients) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| if clients.Config.ForceFlag { | ||||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
| return cmdutil.IsSlackHostedProject(ctx, clients) | ||||||||||||||
|
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. praise: Bolt is coming for you |
||||||||||||||
| return cmdutil.IsValidProjectDirectory(clients) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // runEnvAddCommandFunc sets an app environment variable to given values | ||||||||||||||
| func runEnvAddCommandFunc(clients *shared.ClientFactory, cmd *cobra.Command, args []string) error { | ||||||||||||||
| ctx := cmd.Context() | ||||||||||||||
|
|
||||||||||||||
| // Get the workspace from the flag or prompt | ||||||||||||||
| selection, err := appSelectPromptFunc(ctx, clients, prompts.ShowHostedOnly, prompts.ShowInstalledAppsOnly) | ||||||||||||||
| selection, err := appSelectPromptFunc(ctx, clients, prompts.ShowAllEnvironments, prompts.ShowInstalledAppsOnly) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -127,27 +124,40 @@ func runEnvAddCommandFunc(clients *shared.ClientFactory, cmd *cobra.Command, arg | |||||||||||||
| variableValue = args[1] | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| err = clients.API().AddVariable( | ||||||||||||||
| ctx, | ||||||||||||||
| selection.Auth.Token, | ||||||||||||||
| selection.App.AppID, | ||||||||||||||
| variableName, | ||||||||||||||
| variableValue, | ||||||||||||||
| ) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| // Add the environment variable using either the Slack API method or the | ||||||||||||||
| // project ".env" file depending on the app hosting. | ||||||||||||||
| if !selection.App.IsDev && cmdutil.IsSlackHostedProject(ctx, clients) == nil { | ||||||||||||||
| err = clients.API().AddVariable( | ||||||||||||||
| ctx, | ||||||||||||||
| selection.Auth.Token, | ||||||||||||||
| selection.App.AppID, | ||||||||||||||
| variableName, | ||||||||||||||
| variableValue, | ||||||||||||||
| ) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.EnvAddSuccess) | ||||||||||||||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||||||||||||||
| Emoji: "evergreen_tree", | ||||||||||||||
| Text: "App Environment", | ||||||||||||||
|
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 might follow up to change the section text for these
👾 ramble: I avoided this change for now because this PR is so large!
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. praise: Good call! I like that this PR is keeping ROSI untouched, but we should make a note to follow-up on it adding parity between the two experiences. |
||||||||||||||
| Secondary: []string{ | ||||||||||||||
| fmt.Sprintf("Successfully added \"%s\" as an app environment variable", variableName), | ||||||||||||||
| }, | ||||||||||||||
| })) | ||||||||||||||
| } else { | ||||||||||||||
| err = slackdotenv.Set(clients.Fs, variableName, variableValue) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.EnvAddSuccess) | ||||||||||||||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||||||||||||||
| Emoji: "evergreen_tree", | ||||||||||||||
| Text: "App Environment", | ||||||||||||||
| Secondary: []string{ | ||||||||||||||
| fmt.Sprintf("Successfully added \"%s\" as a project environment variable", variableName), | ||||||||||||||
|
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. 📚 thought: This might behave in unexpected fashion for now since we prompt to select an app but save variables to the project, where all apps have access. I understand we have plans for more nuanced environment variable files and also that these
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. Interesting, this is something that I overlooked when we first proposed the command changes. suggestion(non-blocking): I'm happy with the current approach as phase 1, if you'd like to land the PR, avoid it becoming too large, and iterate. However, I think we need to improve this experience. One of the main value props of the But... if you can't Would checking for the Deno runtime be a way to determine if we need to prompt for an app? If it's Deno then we prompt (local = .env, deploy = API), otherwise we skip the prompt and just use
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. @mwbrooks Thanks for calling out this awkward experience 👾 🔭 I'm confident we can decide to prompt for Deno apps and not Bolt apps, but we might want to follow up to mirror this in the I was hesitant earlier to avoid blocking possible follow ups requiring app selection, but perhaps those explorations make app selection optional! 🚢
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. 🫡 Sounds good! Let's follow-up with exploring how to improve this. |
||||||||||||||
| }, | ||||||||||||||
| })) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| clients.IO.PrintTrace(ctx, slacktrace.EnvAddSuccess) | ||||||||||||||
| clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ | ||||||||||||||
| Emoji: "evergreen_tree", | ||||||||||||||
| Text: "App Environment", | ||||||||||||||
| Secondary: []string{ | ||||||||||||||
| fmt.Sprintf( | ||||||||||||||
| "Successfully added \"%s\" as an environment variable", | ||||||||||||||
| variableName, | ||||||||||||||
| ), | ||||||||||||||
| }, | ||||||||||||||
| })) | ||||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import ( | |
| "github.com/slackapi/slack-cli/internal/slackerror" | ||
| "github.com/slackapi/slack-cli/internal/slacktrace" | ||
| "github.com/slackapi/slack-cli/test/testutil" | ||
| "github.com/spf13/afero" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/mock" | ||
|
|
@@ -46,88 +47,22 @@ var mockApp = types.App{ | |
|
|
||
| func Test_Env_AddCommandPreRun(t *testing.T) { | ||
| tests := map[string]struct { | ||
| mockFlagForce bool | ||
| mockManifestResponse types.SlackYaml | ||
| mockManifestError error | ||
| mockManifestSource config.ManifestSource | ||
| mockWorkingDirectory string | ||
| expectedError error | ||
| }{ | ||
| "continues if the application is hosted on slack": { | ||
| mockManifestResponse: types.SlackYaml{ | ||
| AppManifest: types.AppManifest{ | ||
| Settings: &types.AppSettings{ | ||
| FunctionRuntime: types.SlackHosted, | ||
| }, | ||
| }, | ||
| }, | ||
| mockManifestError: nil, | ||
| mockManifestSource: config.ManifestSourceLocal, | ||
| mockWorkingDirectory: "/slack/path/to/project", | ||
| expectedError: nil, | ||
| }, | ||
| "errors if the application is not hosted on slack": { | ||
| mockManifestResponse: types.SlackYaml{ | ||
| AppManifest: types.AppManifest{ | ||
| Settings: &types.AppSettings{ | ||
| FunctionRuntime: types.Remote, | ||
| }, | ||
| }, | ||
| }, | ||
| mockManifestError: nil, | ||
| mockManifestSource: config.ManifestSourceLocal, | ||
| mockWorkingDirectory: "/slack/path/to/project", | ||
| expectedError: slackerror.New(slackerror.ErrAppNotHosted), | ||
| }, | ||
| "continues if the force flag is used in a project": { | ||
| mockFlagForce: true, | ||
| "continues if the command is run in a project": { | ||
| mockWorkingDirectory: "/slack/path/to/project", | ||
| expectedError: nil, | ||
| }, | ||
| "errors if the project manifest cannot be retrieved": { | ||
| mockManifestResponse: types.SlackYaml{}, | ||
| mockManifestError: slackerror.New(slackerror.ErrSDKHookInvocationFailed), | ||
| mockManifestSource: config.ManifestSourceLocal, | ||
| mockWorkingDirectory: "/slack/path/to/project", | ||
| expectedError: slackerror.New(slackerror.ErrSDKHookInvocationFailed), | ||
| }, | ||
| "errors if the command is not run in a project": { | ||
| mockManifestResponse: types.SlackYaml{}, | ||
| mockManifestError: slackerror.New(slackerror.ErrSDKHookNotFound), | ||
| mockWorkingDirectory: "", | ||
| expectedError: slackerror.New(slackerror.ErrInvalidAppDirectory), | ||
| }, | ||
| "errors if the manifest source is set to remote": { | ||
| mockManifestSource: config.ManifestSourceRemote, | ||
| mockWorkingDirectory: "/slack/path/to/project", | ||
| expectedError: slackerror.New(slackerror.ErrAppNotHosted), | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| clientsMock := shared.NewClientsMock() | ||
| manifestMock := &app.ManifestMockObject{} | ||
| manifestMock.On( | ||
| "GetManifestLocal", | ||
| mock.Anything, | ||
| mock.Anything, | ||
| mock.Anything, | ||
| ).Return( | ||
| tc.mockManifestResponse, | ||
| tc.mockManifestError, | ||
| ) | ||
| clientsMock.AppClient.Manifest = manifestMock | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On( | ||
| "GetManifestSource", | ||
| mock.Anything, | ||
| ).Return( | ||
| tc.mockManifestSource, | ||
| nil, | ||
| ) | ||
| clientsMock.Config.ProjectConfig = projectConfigMock | ||
| clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(cf *shared.ClientFactory) { | ||
| cf.Config.ForceFlag = tc.mockFlagForce | ||
| cf.SDKConfig.WorkingDirectory = tc.mockWorkingDirectory | ||
| }) | ||
| cmd := NewEnvAddCommand(clients) | ||
|
|
@@ -146,7 +81,7 @@ func Test_Env_AddCommand(t *testing.T) { | |
| "add a variable using arguments": { | ||
| CmdArgs: []string{"ENV_NAME", "ENV_VALUE"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddCommandMocks(ctx, cm, cf) | ||
| setupEnvAddHostedMocks(ctx, cm, cf) | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| cm.API.AssertCalled( | ||
|
|
@@ -170,7 +105,7 @@ func Test_Env_AddCommand(t *testing.T) { | |
| "provide a variable name by argument and value by prompt": { | ||
| CmdArgs: []string{"ENV_NAME"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddCommandMocks(ctx, cm, cf) | ||
| setupEnvAddHostedMocks(ctx, cm, cf) | ||
| cm.IO.On( | ||
| "PasswordPrompt", | ||
| mock.Anything, | ||
|
|
@@ -201,7 +136,7 @@ func Test_Env_AddCommand(t *testing.T) { | |
| "provide a variable name by argument and value by flag": { | ||
| CmdArgs: []string{"ENV_NAME", "--value", "example_value"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddCommandMocks(ctx, cm, cf) | ||
| setupEnvAddHostedMocks(ctx, cm, cf) | ||
| cm.IO.On( | ||
| "PasswordPrompt", | ||
| mock.Anything, | ||
|
|
@@ -232,7 +167,7 @@ func Test_Env_AddCommand(t *testing.T) { | |
| "provide both variable name and value by prompt": { | ||
| CmdArgs: []string{}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddCommandMocks(ctx, cm, cf) | ||
| setupEnvAddHostedMocks(ctx, cm, cf) | ||
| cm.IO.On( | ||
| "InputPrompt", | ||
| mock.Anything, | ||
|
|
@@ -269,24 +204,108 @@ func Test_Env_AddCommand(t *testing.T) { | |
| ) | ||
| }, | ||
| }, | ||
| "add a numeric variable using prompts to the .env file": { | ||
| CmdArgs: []string{}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddDotenvMocks(ctx, cm, cf) | ||
| cm.IO.On( | ||
| "InputPrompt", | ||
| mock.Anything, | ||
| "Variable name", | ||
| mock.Anything, | ||
| ).Return( | ||
| "PORT", | ||
| nil, | ||
| ) | ||
| cm.IO.On( | ||
| "PasswordPrompt", | ||
| mock.Anything, | ||
| "Variable value", | ||
| iostreams.MatchPromptConfig(iostreams.PasswordPromptConfig{ | ||
| Flag: cm.Config.Flags.Lookup("value"), | ||
| }), | ||
| ).Return( | ||
| iostreams.PasswordPromptResponse{ | ||
| Prompt: true, | ||
| Value: "3000", | ||
| }, | ||
| nil, | ||
| ) | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| cm.API.AssertNotCalled(t, "AddVariable") | ||
| content, err := afero.ReadFile(cm.Fs, ".env") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "PORT=3000\n", string(content)) | ||
| }, | ||
| }, | ||
| "add a variable to the .env file for non-hosted app": { | ||
|
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. praise: Love seeing the new test use-case! |
||
| CmdArgs: []string{"NEW_VAR", "new_value"}, | ||
| Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| setupEnvAddDotenvMocks(ctx, cm, cf) | ||
| err := afero.WriteFile(cf.Fs, ".env", []byte("# Config\nEXISTING=value\n"), 0600) | ||
| assert.NoError(t, err) | ||
| }, | ||
| ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) { | ||
| cm.API.AssertNotCalled(t, "AddVariable") | ||
| content, err := afero.ReadFile(cm.Fs, ".env") | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "# Config\nEXISTING=value\nNEW_VAR=\"new_value\"\n", string(content)) | ||
| }, | ||
| }, | ||
| }, func(cf *shared.ClientFactory) *cobra.Command { | ||
| cmd := NewEnvAddCommand(cf) | ||
| cmd.PreRunE = func(cmd *cobra.Command, args []string) error { return nil } | ||
| return cmd | ||
| }) | ||
| } | ||
|
|
||
| // setupEnvAddCommandMocks prepares common mocks for these tests | ||
| func setupEnvAddCommandMocks(ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| // setupEnvAddHostedMocks prepares common mocks for hosted app tests | ||
| func setupEnvAddHostedMocks(ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig = hooks.NewSDKConfigMock() | ||
| cm.AddDefaultMocks() | ||
| _ = cf.AppClient().SaveDeployed(ctx, mockApp) | ||
|
|
||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| appSelectMock.On("AppSelectPrompt", mock.Anything, mock.Anything, prompts.ShowHostedOnly, prompts.ShowInstalledAppsOnly).Return(prompts.SelectedApp{Auth: mockAuth, App: mockApp}, nil) | ||
| appSelectMock.On("AppSelectPrompt", mock.Anything, mock.Anything, prompts.ShowAllEnvironments, prompts.ShowInstalledAppsOnly).Return(prompts.SelectedApp{Auth: mockAuth, App: mockApp}, nil) | ||
|
|
||
| cm.Config.Flags.String("value", "", "mock value flag") | ||
|
|
||
| cm.API.On("AddVariable", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) | ||
|
|
||
| manifestMock := &app.ManifestMockObject{} | ||
| manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return( | ||
| types.SlackYaml{ | ||
| AppManifest: types.AppManifest{ | ||
| Settings: &types.AppSettings{ | ||
| FunctionRuntime: types.SlackHosted, | ||
| }, | ||
| }, | ||
| }, | ||
| nil, | ||
| ) | ||
| cm.AppClient.Manifest = manifestMock | ||
| projectConfigMock := config.NewProjectConfigMock() | ||
| projectConfigMock.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil) | ||
| cm.Config.ProjectConfig = projectConfigMock | ||
| cf.SDKConfig.WorkingDirectory = "/slack/path/to/project" | ||
| } | ||
|
|
||
| // setupEnvAddDotenvMocks prepares common mocks for non-hosted (dotenv) app tests | ||
| func setupEnvAddDotenvMocks(_ context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { | ||
| cf.SDKConfig = hooks.NewSDKConfigMock() | ||
| cm.AddDefaultMocks() | ||
|
|
||
| mockDevApp := types.App{ | ||
| TeamID: "T1", | ||
| TeamDomain: "team1", | ||
| AppID: "A0123456789", | ||
| IsDev: true, | ||
| } | ||
| appSelectMock := prompts.NewAppSelectMock() | ||
| appSelectPromptFunc = appSelectMock.AppSelectPrompt | ||
| appSelectMock.On("AppSelectPrompt", mock.Anything, mock.Anything, prompts.ShowAllEnvironments, prompts.ShowInstalledAppsOnly).Return(prompts.SelectedApp{Auth: mockAuth, App: mockDevApp}, nil) | ||
|
|
||
| cm.Config.Flags.String("value", "", "mock value flag") | ||
| } | ||
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: Seems like a nice time to update the Usage to be more accurate?