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
6 changes: 6 additions & 0 deletions .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ for name, tc := range tests {
}
```

### Error Handling

- Wrap errors returned across package boundaries with `slackerror.Wrap(err, slackerror.ErrCode)` so they carry a structured error code
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: This is motivated in ongoing changes with encouragement and optimism for checks from the wonderful wrapcheck lint:

Checks that errors returned from external packages are wrapped.

🔗 https://golangci-lint.run/docs/linters/configuration/#wrapcheck

- Register new error codes in `internal/slackerror/errors.go`: add a constant and an entry in `ErrorCodeMap`
- Error codes are alphabetically ordered in both the constants block and `ErrorCodeMap`

## Version Management

Versions use semantic versioning with git tags (format: `v*.*.*`).
Expand Down
1 change: 1 addition & 0 deletions cmd/platform/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ func deployHook(ctx context.Context, clients *shared.ClientFactory) error {
// so we instantiate the default here.
shell := hooks.HookExecutorDefaultProtocol{
IO: clients.IO,
Fs: clients.Fs,
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: Standalone protocol setups must define fs to access .env but we don't error otherwise. AFAICT this is required for just the deploy and run commands.

}
if _, err := shell.Execute(ctx, hookExecOpts); err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion cmd/platform/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ func TestDeployCommand_DeployHook(t *testing.T) {

clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig = sdkConfigMock
clients.HookExecutor = hooks.GetHookExecutor(clientsMock.IO, sdkConfigMock)
clients.HookExecutor = hooks.GetHookExecutor(clientsMock.IO, clients.Fs, sdkConfigMock)
})
cmd := NewDeployCommand(clients)
cmd.PreRunE = func(cmd *cobra.Command, args []string) error { return nil }
Expand Down
14 changes: 0 additions & 14 deletions internal/config/dotenv.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,10 @@ package config
import (
"strings"

"github.com/joho/godotenv"
"github.com/slackapi/slack-cli/internal/version"
"github.com/spf13/afero"
)

// GetDotEnvFileVariables collects only the variables in the .env file
func (c *Config) GetDotEnvFileVariables() (map[string]string, error) {
variables := map[string]string{}
file, err := afero.ReadFile(c.fs, ".env")
if err != nil && !c.os.IsNotExist(err) {
return variables, err
}
return godotenv.UnmarshalBytes(file)
}
Comment on lines -25 to -33
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: This is moved to our own slackdotenv package to avoid odd dependencies of config and to avoid duplicate logic!


// LoadEnvironmentVariables sets flags based on their environment variable value
//
// Note: Values are not loaded from the .env file. Use: `GetDotEnvFileVariables`
func (c *Config) LoadEnvironmentVariables() error {
// Skip when dependencies are not configured
if c.os == nil {
Expand Down
38 changes: 0 additions & 38 deletions internal/config/dotenv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,47 +18,9 @@ import (
"testing"

"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
)

func Test_DotEnv_GetDotEnvFileVariables(t *testing.T) {
tests := map[string]struct {
globalVariableName string
globalVariableValue string
localEnvFile string
expectedValues map[string]string
}{
"environment file variables are read": {
localEnvFile: "SLACK_VARIABLE=12\n",
expectedValues: map[string]string{"SLACK_VARIABLE": "12"},
},
"variable casing is preserved on load": {
localEnvFile: "secret_Token=Key123!\n",
expectedValues: map[string]string{"secret_Token": "Key123!"},
},
"global environment variables are ignored": {
globalVariableName: "SLACK_VARIABLE",
globalVariableValue: "12",
expectedValues: map[string]string{},
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
fs := slackdeps.NewFsMock()
os := slackdeps.NewOsMock()
os.AddDefaultMocks()
os.Setenv(tc.globalVariableName, tc.globalVariableValue)
err := afero.WriteFile(fs, ".env", []byte(tc.localEnvFile), 0600)
assert.NoError(t, err)
config := NewConfig(fs, os)
variables, err := config.GetDotEnvFileVariables()
assert.NoError(t, err)
assert.Equal(t, tc.expectedValues, variables)
})
}
}

func Test_DotEnv_LoadEnvironmentVariables(t *testing.T) {
tableTests := map[string]struct {
envName string
Expand Down
4 changes: 3 additions & 1 deletion internal/hooks/hook_executor_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,20 @@ import (

"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
)

// HookExecutorDefaultProtocol uses the original protocol between the CLI and the SDK where diagnostic info
// and hook responses come in via stdout. Data outside the expected JSON payload is ignored, with the
// exception of the 'start' hook, for which it is printed.
type HookExecutorDefaultProtocol struct {
IO iostreams.IOStreamer
Fs afero.Fs
}

// Execute processes the data received by the SDK.
func (e *HookExecutorDefaultProtocol) Execute(ctx context.Context, opts HookExecOpts) (string, error) {
cmdArgs, cmdArgVars, cmdEnvVars, err := processExecOpts(opts)
cmdArgs, cmdArgVars, cmdEnvVars, err := processExecOpts(ctx, opts, e.Fs, e.IO)
if err != nil {
return "", err
}
Expand Down
27 changes: 27 additions & 0 deletions internal/hooks/hook_executor_default_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -80,6 +81,31 @@ func Test_Hook_Execute_Default_Protocol(t *testing.T) {
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"dotenv vars and hook vars are loaded into the environment": {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Env: map[string]string{
"OPTS_VAR": "from_opts",
},
Exec: &MockExec{
mockCommand: &MockCommand{
MockStdout: []byte("test output"),
Err: nil,
},
},
},
handler: func(t *testing.T, ctx context.Context, executor HookExecutor, opts HookExecOpts) {
// Write a .env file to the mock filesystem
e := executor.(*HookExecutorDefaultProtocol)
_ = afero.WriteFile(e.Fs, ".env", []byte("DOTENV_VAR=from_dotenv\n"), 0600)

response, err := executor.Execute(ctx, opts)
require.Equal(t, "test output", response)
require.NoError(t, err)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `DOTENV_VAR=from_dotenv`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `OPTS_VAR=from_opts`)
},
},
"failed execution": {
opts: HookExecOpts{
Hook: HookScript{Command: "boom", Name: "sadpath"},
Expand Down Expand Up @@ -156,6 +182,7 @@ func Test_Hook_Execute_Default_Protocol(t *testing.T) {
ios.AddDefaultMocks()
hookExecutor := &HookExecutorDefaultProtocol{
IO: ios,
Fs: afero.NewMemMapFs(),
}
if tc.handler != nil {
tc.handler(t, ctx, hookExecutor, tc.opts)
Expand Down
4 changes: 3 additions & 1 deletion internal/hooks/hook_executor_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,23 @@ import (

"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
)

// HookExecutorMessageBoundaryProtocol uses a protocol between the CLI and the SDK where diagnostic info
// and hook responses come in via stdout, and hook responses are wrapped in a string denoting the
// message boundary. Only one message payload can be received.
type HookExecutorMessageBoundaryProtocol struct {
IO iostreams.IOStreamer
Fs afero.Fs
}

// generateBoundary is a function for creating boundaries that can be mocked
var generateBoundary = generateMD5FromRandomString

// Execute processes the data received by the SDK.
func (e *HookExecutorMessageBoundaryProtocol) Execute(ctx context.Context, opts HookExecOpts) (string, error) {
cmdArgs, cmdArgVars, cmdEnvVars, err := processExecOpts(opts)
cmdArgs, cmdArgVars, cmdEnvVars, err := processExecOpts(ctx, opts, e.Fs, e.IO)
if err != nil {
return "", err
}
Expand Down
30 changes: 30 additions & 0 deletions internal/hooks/hook_executor_v2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -41,6 +42,7 @@ func mockBoundaryStringGenerator() string {
func Test_Hook_Execute_V2_Protocol(t *testing.T) {
tests := map[string]struct {
opts HookExecOpts
setup func(afero.Fs)
check func(*testing.T, string, error, ExecInterface)
}{
"error if hook command unavailable": {
Expand Down Expand Up @@ -132,6 +134,29 @@ func Test_Hook_Execute_V2_Protocol(t *testing.T) {
)
},
},
"dotenv vars and hook vars are loaded into the environment": {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Env: map[string]string{
"OPTS_VAR": "from_opts",
},
Exec: &MockExec{
mockCommand: &MockCommand{
MockStdout: []byte(mockBoundaryString + `{"ok": true}` + mockBoundaryString),
Err: nil,
},
},
},
setup: func(fs afero.Fs) {
_ = afero.WriteFile(fs, ".env", []byte("DOTENV_VAR=from_dotenv\n"), 0600)
},
check: func(t *testing.T, response string, err error, mockExec ExecInterface) {
require.NoError(t, err)
require.Equal(t, `{"ok": true}`, response)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `DOTENV_VAR=from_dotenv`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `OPTS_VAR=from_opts`)
},
},
"fail to parse payload due to improper boundary strings": {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Expand Down Expand Up @@ -176,8 +201,13 @@ func Test_Hook_Execute_V2_Protocol(t *testing.T) {
config := config.NewConfig(fs, os)
ios := iostreams.NewIOStreamsMock(config, fs, os)
ios.AddDefaultMocks()
memFs := afero.NewMemMapFs()
hookExecutor := &HookExecutorMessageBoundaryProtocol{
IO: ios,
Fs: memFs,
}
if tc.setup != nil {
tc.setup(memFs)
}
response, err := hookExecutor.Execute(ctx, tc.opts)
tc.check(t, response, err, tc.opts.Exec)
Expand Down
16 changes: 6 additions & 10 deletions internal/hooks/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,34 @@ package hooks

import (
"context"
"os"
"strings"

"github.com/slackapi/slack-cli/internal/goutils"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/spf13/afero"
)

type HookExecutor interface {
Execute(ctx context.Context, opts HookExecOpts) (response string, err error)
}

func GetHookExecutor(ios iostreams.IOStreamer, cfg SDKCLIConfig) HookExecutor {
func GetHookExecutor(ios iostreams.IOStreamer, fs afero.Fs, cfg SDKCLIConfig) HookExecutor {
protocol := cfg.Config.SupportedProtocols.Preferred()
switch protocol {
case HookProtocolV2:
return &HookExecutorMessageBoundaryProtocol{
IO: ios,
Fs: fs,
}
default:
return &HookExecutorDefaultProtocol{
IO: ios,
Fs: fs,
}
}
}

func processExecOpts(opts HookExecOpts) ([]string, []string, []string, error) {
func processExecOpts(ctx context.Context, opts HookExecOpts, fs afero.Fs, io iostreams.IOStreamer) ([]string, []string, []string, error) {
cmdStr, err := opts.Hook.Get()
if err != nil {
return []string{}, []string{}, []string{}, err
Expand All @@ -53,13 +55,7 @@ func processExecOpts(opts HookExecOpts) ([]string, []string, []string, error) {
var cmdArgVars = cmdArgs[1:] // omit the first item because that is the command name
cmdArgVars = append(cmdArgVars, goutils.MapToStringSlice(opts.Args, "--")...)

// Whatever cmd.Env is set to will be the ONLY environment variables that the `cmd` will have access to when it runs.
// To avoid removing any environment variables that are set in the current environment, we first set the cmd.Env to the current environment.
// before adding any new environment variables.
var cmdEnvVars = os.Environ()
for name, value := range opts.Env {
cmdEnvVars = append(cmdEnvVars, name+"="+value)
}
cmdEnvVars := opts.ShellEnv(ctx, fs, io)

return cmdArgs, cmdArgVars, cmdEnvVars, nil
}
2 changes: 1 addition & 1 deletion internal/hooks/hooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func Test_Hooks_GetHookExecutor(t *testing.T) {
io := iostreams.NewIOStreamsMock(config, fs, os)
sdkConfig := NewSDKConfigMock()
sdkConfig.Config.SupportedProtocols = tc.protocolVersions
hookExecutor := GetHookExecutor(io, sdkConfig)
hookExecutor := GetHookExecutor(io, fs, sdkConfig)
require.IsType(t, tc.expectedType, hookExecutor)
})
}
Expand Down
43 changes: 43 additions & 0 deletions internal/hooks/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@
package hooks

import (
"context"
"fmt"
"io"
"os"
"os/exec"
"runtime"
"strings"

"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/slackdotenv"
"github.com/spf13/afero"
)

// ExecInterface is an interface for running shell commands in the OS
Expand Down Expand Up @@ -92,3 +97,41 @@ type HookExecOpts struct {
Stderr io.Writer
Exec ExecInterface
}

// ShellEnv builds the environment variables for a hook command.
func (opts HookExecOpts) ShellEnv(ctx context.Context, fs afero.Fs, io iostreams.IOStreamer) []string {
// Gather environment variables saved to the project ".env" file
dotEnv, err := slackdotenv.Read(fs)
if err != nil {
io.PrintWarning(ctx, "%s", err)
}
if len(dotEnv) > 0 {
keys := make([]string, 0, len(dotEnv))
for k := range dotEnv {
keys = append(keys, k)
}
io.PrintDebug(ctx, "Loaded variables from .env file: %s", strings.Join(keys, ", "))
}

// Whatever cmd.Env is set to will be the ONLY environment variables that the `cmd` will have access to when it runs.
//
// Order of precedence from lowest to highest:
// 1. Provided "opts.Env" variables
// 2. Saved ".env" file
// 3. Existing shell environment
//
// > Each entry is of the form "key=value".
// > ...
// > If Env contains duplicate environment keys, only the last value in the slice for each duplicate key is used.
//
// https://pkg.go.dev/os/exec#Cmd.Env
var cmdEnvVars []string
for name, value := range opts.Env {
cmdEnvVars = append(cmdEnvVars, name+"="+value)
}
for k, v := range dotEnv {
cmdEnvVars = append(cmdEnvVars, k+"="+v)
}
cmdEnvVars = append(cmdEnvVars, os.Environ()...)
return cmdEnvVars
}
Loading
Loading