Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"Bash(git log:*)",
"Bash(git status:*)",
"Bash(go build:*)",
"Bash(go doc:*)",
"Bash(go mod graph:*)",
"Bash(go mod tidy:*)",
"Bash(go mod tidy:*)",
Expand Down
80 changes: 45 additions & 35 deletions cmd/env/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -31,14 +32,17 @@ import (
func NewEnvAddCommand(clients *shared.ClientFactory) *cobra.Command {
cmd := &cobra.Command{
Use: "add <name> <value> [flags]",
Copy link
Copy Markdown
Member

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?

Suggested change
Use: "add <name> <value> [flags]",
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
"the \".env\" file. This includes the \"run\" command.",
"",
"The \"deploy\" command gathers environment variables from the \".env\" file as well",
`the ".env" file. This includes the "run" command.`,
"",
`The "deploy" command gathers environment variables from the ".env" file as well`,

"unless the app is using ROSI features.",
}, "\n"),
Example: style.ExampleCommandsf([]style.ExampleCommand{
{
Expand Down Expand Up @@ -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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Bolt is coming for you env! ⚡

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
}
Expand Down Expand Up @@ -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",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 note: We might follow up to change the section text for these env commands for outputs that match:

  • 🌲 Environment Add
  • 🌲 Environment List
  • 🌲 Environment Remove

👾 ramble: I avoided this change for now because this PR is so large!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 .env file behaviors are understood when developing so I don't know if this requires more of a callout!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 env add command is to set your app's third-party API Tokens. However, these tokens need to be set before you can run the app.

But... if you can't env add until you've created an app, then you're in a bit of a pickle. 🐣 You'd need to use the (currently) awkward install command before env add.

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 .env.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 env list command.

I was hesitant earlier to avoid blocking possible follow ups requiring app selection, but perhaps those explorations make app selection optional! 🚢

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
}
167 changes: 93 additions & 74 deletions cmd/env/add_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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")
}
10 changes: 6 additions & 4 deletions cmd/env/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@ func NewCommand(clients *shared.ClientFactory) *cobra.Command {
Aliases: []string{"var", "vars", "variable", "variables"},
Short: "Add, remove, or list environment variables",
Long: strings.Join([]string{
"Add, remove, or list environment variables for apps deployed to Slack managed",
"infrastructure.",
"Add, remove, or list environment variables for the app.",
"",
"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",
"unless the app is using ROSI features.",
"",
`Explore more: {{LinkText "https://docs.slack.dev/tools/slack-cli/guides/using-environment-variables-with-the-slack-cli"}}`,
}, "\n"),
Expand Down
Loading
Loading