feat: load environment variables from '.env' file for hook commands#436
feat: load environment variables from '.env' file for hook commands#436
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 70.27% 70.27% -0.01%
==========================================
Files 220 220
Lines 18498 18534 +36
==========================================
+ Hits 12999 13024 +25
- Misses 4324 4336 +12
+ Partials 1175 1174 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
👾 A few thoughts for wonderful reviewers-
There was a problem hiding this comment.
🗣️ note: The run command uses a separate hook process to handle automatic restarts with file watching so we duplicate some logic here!
| // so we instantiate the default here. | ||
| shell := hooks.HookExecutorDefaultProtocol{ | ||
| IO: clients.IO, | ||
| Fs: clients.Fs, |
There was a problem hiding this comment.
🔭 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.
mwbrooks
left a comment
There was a problem hiding this comment.
🙌🏻 Woohoo, this is so awesome to see landing!
🥾 I think we have a few more steps to tighten things up. I've left a few suggestions in-line around consolidating code and testing edge-cases that users will hit. Below are a few more suggestions.
suggestion(non-blocking): We have some existing dotenv logic in internal/config/dotenv.go. This seems like a reasonable place to put our new parsing logic, so that everything is in one place. Or, rename it to something that feels better to us. This could be a follow-up PR but we should make sure that we don't fragment our dotenv logic.
suggestion: I think we should update the PR title and CHANGELOG description to focus a little more on the use-facing feature. For example: "feat: support loading a dotenv '.env' file for your app"
| require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `YIN=yang`) | ||
| }, | ||
| }, | ||
| "dotenv vars are loaded": { |
There was a problem hiding this comment.
suggestion: Very nice test!
With my head in the weeds of how this all works, I appreciate that you're testing both session environment variables (Env:) and dotenv variables (.env).
My concern is that our future selves (or teammates) but not pick up on this nuance.
Perhaps a comment would help explain that we're testing both sources of environment variables?
| ) | ||
| }, | ||
| }, | ||
| "dotenv vars are loaded": { |
There was a problem hiding this comment.
note: If we decide to add a comment to the table tests for the default hook executor, I suppose we should do it here as well.
| // 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 |
There was a problem hiding this comment.
praise: ❤️ 🖊️ Love the detailed comment for future readers!
| // 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 |
There was a problem hiding this comment.
suggestion: This logic feels important and since it's used in 2 places, I think we should consider DRY'ing up the code and putting the logic into 1 place. For example, internal/config/dotenv.go (existing) or rename the file to internal/slackdotenv if we want something that doesn't name collide with the dotenv dependency.
Additionally, a single function would make future improvements easier. For example, we may want to introduce a --dotenv-overwrite flag and { "dotenv-overwrite": true config value that allow the .env to overwrite session variables. This seems to be a common use-case because most dotenv libraries support it, including our package with godotenv.Overload().
Note: This would also allow us to unit test the scenarios in internal/config/dotenv_test.go instead of in the localserver_test.go and hooks_test.go.
| // Load .env file variables | ||
| dotEnv, err := LoadDotEnv(fs) | ||
| if err != nil { | ||
| io.PrintDebug(ctx, "Warning: failed to parse .env file: %s", err) |
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 .env. The warning is printed into debug, which I don't see. Then I spend a long time debugging my app, wondering why it's not working until I realize that the environment variables are never loaded.
| for k := range dotEnv { | ||
| keys = append(keys, k) | ||
| } | ||
| io.PrintDebug(ctx, "loaded variables from .env file: %s", strings.Join(keys, ", ")) |
There was a problem hiding this comment.
nit: We use capital "Loaded" in localserver.go. Perhaps capitalize this as well?
| io.PrintDebug(ctx, "loaded variables from .env file: %s", strings.Join(keys, ", ")) | |
| io.PrintDebug(ctx, "Loaded variables from .env file: %s", strings.Join(keys, ", ")) |
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func Test_Hooks_LoadDotEnv(t *testing.T) { |
There was a problem hiding this comment.
suggestion: All of these tests are great, but we should add one for malformed .env files as well, I think. Since it's a user-edited file, we're guaranteed to have some malformed files.
| // 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) | ||
| } |
There was a problem hiding this comment.
question: Should we move this logic to internal/config/dotenv.go to consolidate dotenv handling to one place?
Changelog
Summary
This PR loads environment variables from the
.envfile for hook commands. These variables are loaded before each hook command executes to ensure correctness 🌲 ✨We also fix orderings of environment variable precedence within hooks to be:
SLACK_CLI_XOXPtoken provided with theruncommand..env. file.EXPORTor set variables otherwise. This is most important!Preview
demo.mov
Reviewers
A few test cases might be interesting to saved variables:
Notes
We might follow up with similar
.envpatterns and I'm hoping these changes guide decent direction to placement of loading environment variables for hooks!internal/config/dotenvpackage is for internal configurations instead of developer application variables.clients.Config.ManifestEnvattribute is used formanifestandtriggercommands but notrunor thedeploycommands for Bolt apps. I'm unsure that we should continue to support this as we bring enhancement to a project ".env" file itself? Regardless, it wasn't the right rabbit hole...Requirements