Implement env_file directive support#300
Merged
Merged
Conversation
Reviewer's GuideAdds first-class support for dotenv-style env_file directives at both global and command scope, wiring them into config parsing, runtime environment resolution, precedence rules, and tests/docs, while refactoring builtin env handling and tightening some error messages/logging. Sequence diagram for command environment resolution with env_filesequenceDiagram
actor User
participant CLI as Main
participant Exec as Executor
participant Cfg as Config
participant Cmd as Command
participant EnvFilesGlobal as EnvFiles_global
participant EnvFilesCmd as EnvFiles_command
User->>CLI: invoke lets <command>
CLI->>Exec: create Executor with Config
Note over Exec,Cfg: Global env resolution (once per config)
Exec->>Cfg: SetupEnv()
activate Cfg
Cfg->>Cfg: BuiltinEnv(shell)
Cfg->>EnvFilesGlobal: Load(cfg, builtin+Env.Dump())
EnvFilesGlobal-->>Cfg: map globalEnvFileEnv
Cfg->>Cfg: merge Env.Dump() then globalEnvFileEnv into resolvedEnv
Cfg-->>Exec: SetupEnv done
deactivate Cfg
Note over Exec,Cmd: Per-command env resolution
Exec->>Cfg: CommandBuiltinEnv(Cmd, shell, workDir)
Cfg-->>Exec: defaultEnv (builtin lets vars for command)
Exec->>Cmd: GetEnv(*Cfg, defaultEnv)
activate Cmd
Cmd->>Cmd: clone builtinEnv
Cmd->>Cfg: GetEnv() (global resolvedEnv)
Cfg-->>Cmd: globalEnv
Cmd->>Cmd: merge globalEnv into baseEnv
Cmd->>Cmd: Env.Execute(cfg, baseEnv)
Cmd->>EnvFilesCmd: Load(cfg, baseEnv + Env.Dump())
EnvFilesCmd-->>Cmd: map cmdEnvFileEnv
Cmd->>Cmd: resolvedEnv = Env.Dump()
Cmd->>Cmd: overlay cmdEnvFileEnv on resolvedEnv
Cmd-->>Exec: cmdEnv (clone of resolvedEnv)
deactivate Cmd
Exec->>Exec: merge checksum vars, -E/--env, options
Exec->>Exec: construct final process env
Exec-->>User: run command with resolved environment
Class diagram for env_file support in config and commandclassDiagram
class Config {
+string FilePath
+string Before
+string Init
+Envs Env
+EnvFiles EnvFiles
+string Version
+map~string,string~ resolvedEnv
+bool isMixin
+string DotLetsDir
+string ChecksumsDir
+map~string,*Command~ Commands
+UnmarshalYAML(unmarshal)
+mergeMixin(mixin *Config) error
+readMixins(mixins []*Mixin) error
+GetEnv() map~string,string~
+SetupEnv() error
+BuiltinEnv(shell string) map~string,string~
+CommandBuiltinEnv(command *Command, shell string, workDir string) map~string,string~
}
class Command {
+string Name
+[]string Args
+string WorkDir
+string Description
+Envs Env
+EnvFiles EnvFiles
+string Shell
+string Docopts
+bool SkipDocopts
+map~string,string~ Options
+map~string,string~ resolvedEnv
+UnmarshalYAML(unmarshal) error
+GetEnv(cfg Config, builtinEnv map~string,string~) (map~string,string~, error)
+Clone() *Command
}
class EnvFile {
+string Name
+bool Required
+UnmarshalYAML(unmarshal) error
}
class EnvFiles {
+[]EnvFile Items
+map~string,string~ loaded
+bool ready
+UnmarshalYAML(node *yaml.Node) error
+Clone() *EnvFiles
+Append(other *EnvFiles)
+Load(cfg Config, envMap map~string,string~) (map~string,string~, error)
}
class Envs {
+map~string,*Env~ Mapping
+UnmarshalYAML(node *yaml.Node) error
+Execute(cfg Config, baseEnv map~string,string~) error
+Dump() map~string,string~
+Clone() *Envs
+Merge(other *Envs)
}
class Executor {
-Config cfg
+setupEnv(osCmd *exec.Cmd, command *config.Command, shell string) error
}
Config "1" o-- "many" Command : Commands
Config "1" *-- Envs : Env
Config "1" *-- EnvFiles : EnvFiles
Command "1" *-- Envs : Env
Command "1" *-- EnvFiles : EnvFiles
EnvFiles "1" *-- "many" EnvFile : Items
Executor ..> Config : uses
Executor ..> Command : executes
Command ..> Config : GetEnv
EnvFiles ..> Config : Load
Command ..> EnvFiles : EnvFiles
Config ..> EnvFiles : EnvFiles
Config ..> Envs : Env
Command ..> Envs : Env
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/config/config/command.go" line_range="138-147" />
<code_context>
-func (c *Command) GetEnv(cfg Config) (map[string]string, error) {
- if err := c.Env.Execute(cfg, cfg.GetEnv()); err != nil {
+func (c *Command) GetEnv(cfg Config, builtinEnv map[string]string) (map[string]string, error) {
+ if c.resolvedEnv != nil {
+ return cloneMap(c.resolvedEnv), nil
+ }
+
+ baseEnv := cloneMap(builtinEnv)
+ if baseEnv == nil {
+ baseEnv = make(map[string]string)
+ }
+ for key, value := range cfg.GetEnv() {
+ baseEnv[key] = value
+ }
+
+ if err := c.Env.Execute(cfg, baseEnv); err != nil {
return nil, err
}
- return c.Env.Dump(), nil
+ filenameEnv := cloneMap(baseEnv)
+ for key, value := range c.Env.Dump() {
+ filenameEnv[key] = value
+ }
+
+ envFileEnv, err := c.EnvFiles.Load(cfg, filenameEnv)
+ if err != nil {
+ return nil, fmt.Errorf("failed to resolve env_file for command '%s': %w", c.Name, err)
+ }
+
+ c.resolvedEnv = c.Env.Dump()
+ for key, value := range envFileEnv {
+ c.resolvedEnv[key] = value
+ }
+
+ return cloneMap(c.resolvedEnv), nil
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Command env caching ignores builtinEnv, so repeated runs with different args/dirs/shell reuse incorrect env
`c.resolvedEnv` is cached per `Command` instance, but the computed env depends on `builtinEnv` (`LETS_COMMAND_ARGS`, `LETS_COMMAND_WORK_DIR`, `LETS_SHELL`, etc.). Re-invoking the same `Command` with different args/dirs/shell will still return a clone of the first computed env, ignoring later `builtinEnv` values.
To avoid stale envs across invocations, either remove caching at the `Command` level or incorporate the relevant `builtinEnv` values into the cache key / `resolvedEnv` so each distinct invocation gets its own env.
</issue_to_address>
### Comment 2
<location path="internal/config/config/env_file.go" line_range="107-116" />
<code_context>
+func (e *EnvFiles) Load(cfg Config, envMap map[string]string) (map[string]string, error) {
</code_context>
<issue_to_address>
**issue (bug_risk):** EnvFiles.Load caches results without considering cfg or envMap, which can lead to incorrect reuse
The cache in `EnvFiles.Load` is keyed only by the `EnvFiles` instance, but the result also depends on `cfg.WorkDir` and `envMap` (via `expandWithEnv`). Reusing the same `EnvFiles` with different configs/envMaps will incorrectly return the first `loaded` result.
Either remove this caching, or scope it to a specific `(cfg, envMap)` context (or move caching to a higher layer where those inputs are stable).
</issue_to_address>
### Comment 3
<location path="internal/config/config/env_file.go" line_range="26-35" />
<code_context>
+ ready bool
+}
+
+func (e *EnvFile) UnmarshalYAML(unmarshal func(interface{}) error) error {
+ var filename string
+ if err := unmarshal(&filename); err == nil {
+ e.Name = normalizeEnvFilename(filename)
+ e.Required = !isOptionalEnvFilename(filename)
+ if e.Name == "" {
+ return errors.New("env_file name can not be empty")
+ }
+
+ return nil
+ }
+
+ var raw struct {
+ Name string
+ Required *bool
+ }
+ if err := unmarshal(&raw); err != nil {
+ return err
+ }
+
+ if raw.Name == "" {
+ return errors.New("env_file name can not be empty")
+ }
+
+ e.Name = raw.Name
+ e.Required = true
+ if raw.Required != nil {
+ e.Required = *raw.Required
+ }
+
+ return nil
+}
+
</code_context>
<issue_to_address>
**question:** Scalar and map env_file forms handle optional filenames differently; consider normalizing behavior
In the scalar form, `normalizeEnvFilename`/`isOptionalEnvFilename` treat a leading `-` as “optional” and strip it from the stored name. In the map form, `raw.Name` is used verbatim and `Required` defaults to true, so `name: -file.env` stays `-file.env` and is not treated as optional. If the map form is meant to mirror the scalar semantics, consider applying the same normalization/optionality logic to `raw.Name`.
</issue_to_address>
### Comment 4
<location path="internal/config/config/env_file_test.go" line_range="36" />
<code_context>
+ }
+}
+
+func TestParseEnvFiles(t *testing.T) {
+ t.Run("parses mixed env_file forms", func(t *testing.T) {
+ text := dedent.Dedent(`
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for scalar and map `env_file` forms and invalid top-level kinds
`TestParseEnvFiles` currently only exercises the sequence form and the error for a map item missing `name`. To improve coverage, please also add:
- A scalar case (`env_file: .env`) to confirm single-string values parse correctly.
- A valid map case (e.g. `env_file: { name: .env.prod, required: false }`) to verify `required` is respected.
- An invalid top-level kind (e.g. integer or boolean) to assert the `env_file must be a string, map, or sequence` error from `EnvFiles.UnmarshalYAML` is triggered.
Suggested implementation:
```golang
func TestParseEnvFiles(t *testing.T) {
t.Run("parses scalar env_file", func(t *testing.T) {
text := dedent.Dedent(`
env_file: .env
`)
var raw struct {
EnvFiles *EnvFiles `yaml:"env_file"`
}
if err := yaml.Unmarshal([]byte(text), &raw); err != nil {
t.Fatalf("unexpected error unmarshalling scalar env_file: %v", err)
}
if raw.EnvFiles == nil {
t.Fatalf("expected EnvFiles to be populated for scalar env_file")
}
if len(*raw.EnvFiles) != 1 {
t.Fatalf("expected 1 env_file entry, got %d", len(*raw.EnvFiles))
}
// Adjust field names if they differ in EnvFile
if (*raw.EnvFiles)[0].Name != ".env" {
t.Fatalf("expected env_file name '.env', got %q", (*raw.EnvFiles)[0].Name)
}
})
t.Run("parses map env_file", func(t *testing.T) {
text := dedent.Dedent(`
env_file:
name: .env.prod
required: false
`)
var raw struct {
EnvFiles *EnvFiles `yaml:"env_file"`
}
if err := yaml.Unmarshal([]byte(text), &raw); err != nil {
t.Fatalf("unexpected error unmarshalling map env_file: %v", err)
}
if raw.EnvFiles == nil {
t.Fatalf("expected EnvFiles to be populated for map env_file")
}
if len(*raw.EnvFiles) != 1 {
t.Fatalf("expected 1 env_file entry, got %d", len(*raw.EnvFiles))
}
ef := (*raw.EnvFiles)[0]
// Adjust field names if they differ in EnvFile
if ef.Name != ".env.prod" {
t.Fatalf("expected env_file name '.env.prod', got %q", ef.Name)
}
if ef.Required {
t.Fatalf("expected env_file required=false to be respected")
}
})
t.Run("errors on invalid top-level kind", func(t *testing.T) {
text := dedent.Dedent(`
env_file: 123
`)
var raw struct {
EnvFiles *EnvFiles `yaml:"env_file"`
}
err := yaml.Unmarshal([]byte(text), &raw)
if err == nil {
t.Fatalf("expected error unmarshalling invalid top-level env_file kind, got nil")
}
if !strings.Contains(err.Error(), "env_file must be a string, map, or sequence") {
t.Fatalf("expected env_file kind error, got: %v", err)
}
})
t.Run("parses mixed env_file forms", func(t *testing.T) {
text := dedent.Dedent(`
env_file:
- .env
- -.env.local
- name: .env.prod
required: false
`)
var raw struct {
EnvFiles *EnvFiles `yaml:"env_file"`
}
```
1. Ensure `yaml` and `strings` are imported in this test file (they likely already are for `yaml`, but `strings` may need to be added):
- `gopkg.in/yaml.v3` (or whatever YAML package the file already uses).
- `"strings"` for the error substring check.
2. The code above assumes:
- `EnvFiles` is a slice-like type (`[]EnvFile`) so that `len(*raw.EnvFiles)` and index access are valid.
- The element type has fields `Name string` and `Required bool`.
If the actual type or field names differ (e.g. `Path` instead of `Name`, or `Optional` instead of `Required`), update the field accesses and assertions accordingly.
3. Keep the remainder of the existing `"parses mixed env_file forms"` subtest body (assertions, error checks, etc.) after the shown snippet so current coverage is preserved.
</issue_to_address>
### Comment 5
<location path="internal/config/config/env_file_test.go" line_range="90" />
<code_context>
+ })
+}
+
+func TestEnvFilesLoad(t *testing.T) {
+ workDir := t.TempDir()
+ writeFixtureFile(t, workDir, ".env.first", "VALUE=first\n")
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to cover filename interpolation precedence between provided env map and process environment
Current `TestEnvFilesLoad` cases only pass `nil` for the env map, so they don’t cover precedence between the explicit env map and `os.Environ()` during filename interpolation. Please add a subtest that:
- Sets `os.Setenv("TARGET", "from-os")`.
- Calls `Load` with an env map containing `TARGET=from-map` and a pattern like `.env.${TARGET}`.
- Asserts the selected filename resolves using `from-map`, not `from-os`.
This will lock in the intended precedence behavior for filename expansion.
Suggested implementation:
```golang
cfg := Config{WorkDir: workDir}
t.Run("env map overrides process env for filename interpolation", func(t *testing.T) {
t.Setenv("TARGET", "from-os")
envFiles := &EnvFiles{
Items: []EnvFile{
{Name: ".env.${TARGET}", Required: true},
},
}
env := map[string]string{
"TARGET": "from-map",
}
ctx := context.Background()
values, err := cfg.LoadEnvFiles(ctx, envFiles, env)
if err != nil {
t.Fatalf("LoadEnvFiles failed: %v", err)
}
if got, want := values["VALUE"], "first"; got != want {
t.Fatalf("unexpected VALUE: got %q, want %q", got, want)
}
})
t.Run("later files override earlier files", func(t *testing.T) {
```
1. Ensure `context` is imported at the top of `env_file_test.go`:
- If not present, add `import "context"` (or include it in the existing import block).
2. If the function under test has a different name or signature than `cfg.LoadEnvFiles(ctx, envFiles, env)`, adjust the call accordingly so that:
- The first argument is a `context.Context` (or whatever your existing tests use).
- The second argument is the `EnvFiles` collection.
- The third argument is the explicit env map used for interpolation.
3. The assertion assumes `.env.first` contains `VALUE=first` and that `.env.${TARGET}` is resolved using `TARGET` from the provided env map; if your API returns values in a different structure, adapt the indexing (`values["VALUE"]`) to your actual return type.
</issue_to_address>
### Comment 6
<location path="internal/config/config/env_file_test.go" line_range="165" />
<code_context>
+ })
+}
+
+func TestConfigSetupEnvWithEnvFile(t *testing.T) {
+ workDir := t.TempDir()
+ writeFixtureFile(t, workDir, ".env.global", "FROM_FILE=global-file\nOVERRIDE=from-file\n")
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that subsequent GetEnv calls are stable and do not mutate shared state
Since config env is cached, consider also asserting that a second `cfg.GetEnv()` call returns an equal but distinct map (mutating the first result does not affect the second). This clarifies the contract that callers get an isolated copy while still benefiting from caching.
Suggested implementation:
```golang
package config
import (
"testing"
"github.com/lithammer/dedent"
)
// TestGetEnvReturnsIsolatedCopies verifies that cfg.GetEnv() is stable
// and returns an equal but distinct map on subsequent calls, so that
// mutating the first result does not affect the second or subsequent calls.
func TestGetEnvReturnsIsolatedCopies(t *testing.T) {
workDir := t.TempDir()
cfg := loadConfigFixture(t, workDir, dedent.Dedent(`
shell: bash
env:
FOO: foo
BAR: bar
`))
// First call to GetEnv
env1 := cfg.GetEnv()
if env1["FOO"] != "foo" || env1["BAR"] != "bar" {
t.Fatalf("unexpected env1 contents: %#v", env1)
}
// Second call should return the same logical values
env2 := cfg.GetEnv()
if env2["FOO"] != "foo" || env2["BAR"] != "bar" {
t.Fatalf("unexpected env2 contents: %#v", env2)
}
// The maps should be logically equal
if len(env1) != len(env2) {
t.Fatalf("env1 and env2 length differ: %d vs %d", len(env1), len(env2))
}
for k, v1 := range env1 {
if v2, ok := env2[k]; !ok || v1 != v2 {
t.Fatalf("env mismatch for key %q: env1=%q env2=%q (present=%v)", k, v1, v2, ok)
}
}
// Mutate the first map and ensure it does not affect the second
env1["NEW_FROM_ENV1"] = "value"
delete(env1, "FOO")
if _, ok := env2["NEW_FROM_ENV1"]; ok {
t.Fatalf("env2 unexpectedly saw mutation from env1: %#v", env2)
}
if env2["FOO"] != "foo" {
t.Fatalf("env2[\"FOO\"] changed after mutating env1: %#v", env2)
}
// A third call should also be unaffected by mutations to env1/env2
env3 := cfg.GetEnv()
if env3["FOO"] != "foo" || env3["BAR"] != "bar" {
t.Fatalf("env3 contents unexpectedly changed: %#v", env3)
}
if _, ok := env3["NEW_FROM_ENV1"]; ok {
t.Fatalf("env3 unexpectedly saw mutation from env1: %#v", env3)
}
}
```
1. Ensure the package name `package config` at the top of the new file matches the existing package declaration in `internal/config/config/env_file_test.go` and other tests in this directory; adjust it if the package name is different.
2. This test assumes that `loadConfigFixture` and `writeFixtureFile` (if needed elsewhere) are helper functions already defined in the same package, as in the existing tests; if they are in a different package or have different names, update the calls accordingly.
3. If `github.com/lithammer/dedent` is imported in a common `_test.go` file under the same package, you can remove the explicit import here and rely on that, or adjust the import path if your project uses a local dedent helper instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Greptile SummaryThis PR implements the Key points from the review:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant M as main.go
participant C as Config
participant Cmd as Command
participant EF as EnvFiles
participant GD as godotenv
M->>C: SetupEnv()
C->>C: Env.Execute() — resolve global env sh expressions
C->>C: Build filenameEnv (BuiltinEnv + global Env.Dump())
C->>EF: Load(cfg, filenameEnv)
EF->>EF: expandWithEnv(item.Name, filenameEnv) per file
EF->>GD: Read(resolvedFilePath)
GD-->>EF: map[string]string
EF-->>C: envFileEnv
C->>C: resolvedEnv = Env.Dump() + envFileEnv
M->>M: executor.setupEnv(osCmd, command, shell)
M->>C: CommandBuiltinEnv(command, shell, workDir)
C-->>M: defaultEnv (LETS_* vars)
M->>Cmd: GetEnv(cfg, defaultEnv)
Cmd->>Cmd: baseEnv = defaultEnv + cfg.GetEnv()
Cmd->>Cmd: Env.Execute(cfg, baseEnv) — resolve command env sh
Cmd->>Cmd: filenameEnv = baseEnv + Env.Dump()
Cmd->>EF: Load(cfg, filenameEnv)
EF->>GD: Read(resolvedFilePath)
GD-->>EF: map[string]string
EF-->>Cmd: envFileEnv
Cmd->>Cmd: resolvedEnv = Env.Dump() + envFileEnv
Cmd-->>M: cmdEnv
M->>M: Build final env list (os.Environ + defaultEnv + cfg.GetEnv() + cmdEnv + options + checksums)
M->>M: osCmd.Env = envList
Last reviewed commit: 52c48c8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
kindermax
force-pushed
the
codex/plan-envfile-directive-support
branch
from
52c48c8 to
cd663b4
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Testing
Summary by Sourcery
Add support for loading dotenv-style env files via a new env_file directive at both global and command scope, integrating them into environment resolution and precedence.
New Features:
Enhancements:
Build:
Documentation:
Tests: