feat: load environment variables from '.env' file for hook commands #436
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,29 +19,49 @@ import ( | |||||
| "os" | ||||||
| "strings" | ||||||
|
|
||||||
| "github.com/joho/godotenv" | ||||||
| "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) | ||||||
| } | ||||||
|
|
||||||
| // LoadDotEnv reads and parses a .env file from the working directory using the | ||||||
| // provided filesystem. It returns nil if the file does not exist. | ||||||
| func LoadDotEnv(fs afero.Fs) (map[string]string, error) { | ||||||
| if fs == nil { | ||||||
| return nil, nil | ||||||
| } | ||||||
| file, err := afero.ReadFile(fs, ".env") | ||||||
| if err != nil { | ||||||
| if os.IsNotExist(err) { | ||||||
| return nil, nil | ||||||
| } | ||||||
| return nil, err | ||||||
| } | ||||||
| return godotenv.UnmarshalBytes(file) | ||||||
| } | ||||||
|
Comment on lines
+32
to
+46
Member
There was a problem hiding this comment. question: Should we move this logic to |
||||||
|
|
||||||
| 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(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 | ||||||
|
|
@@ -53,13 +73,39 @@ 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, "--")...) | ||||||
|
|
||||||
| // Load .env file variables | ||||||
| dotEnv, err := LoadDotEnv(fs) | ||||||
| if err != nil { | ||||||
| io.PrintDebug(ctx, "Warning: failed to parse .env file: %s", err) | ||||||
|
Member
There was a problem hiding this comment. question: Should we be more noisy about this warning? The scenario that I think about is that I have a malformed |
||||||
| } | ||||||
| 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, ", ")) | ||||||
|
Member
There was a problem hiding this comment. nit: We use capital "Loaded" in localserver.go. Perhaps capitalize this as well?
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| // 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 | ||||||
|
Comment on lines
+91
to
+100
Member
There was a problem hiding this comment. praise: ❤️ 🖊️ Love the detailed comment for future readers! |
||||||
| 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 cmdArgs, cmdArgVars, cmdEnvVars, nil | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔭 note: Standalone protocol setups must define
fsto access.envbut we don't error otherwise. AFAICT this is required for just thedeployandruncommands.