fix: propagate templates.settings.env to gomplate datasources#2084
Merged
fix: propagate templates.settings.env to gomplate datasources#2084
Conversation
- Update TerraformRunner interface for terraform-exec v0.25.0 (WorkspaceSelect now accepts variadic options) - Downgrade gocloud.dev to v0.41.0 and go-fsimpl to v0.3.1 to maintain compatibility with gomplate/v3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three interconnected bugs prevented `templates.settings.env` from reaching gomplate datasources: mapstructure:"-" tag silently dropped the Env field during encode/decode/merge pipeline, Viper lowercased env var keys (e.g., AWS_PROFILE → aws_profile), and stack manifest env vars were dropped at caller decode sites. Fix extracts env from raw sources before mapstructure drops it, restores env after merge, adds deferred cleanup to prevent env pollution across components, and extends case-sensitive path support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
Contributor
📝 WalkthroughWalkthroughAdds extraction and restoration of template environment variables across config loading and template processing (including Gomplate datasources), extends case-sensitive key handling for templates.settings.env, updates dependency versions/licenses, adjusts Terraform WorkspaceSelect signature, and adds tests and fixtures for env propagation. Changes
Sequence DiagramsequenceDiagram
participant CLI as Atmos CLI
participant Config as Config Loader
participant TmplUtils as Template Utils
participant Gomplate as Gomplate Engine
participant OS as OS Environment
CLI->>Config: LoadConfig(atmos.yaml + stack)
Config->>TmplUtils: Provide raw templates.settings map
TmplUtils->>TmplUtils: extractEnvFromRawMap(raw)
TmplUtils->>TmplUtils: setEnvVarsWithRestore(extractedEnv)
TmplUtils->>OS: Set env vars (via OS)
TmplUtils->>Gomplate: Execute templates/datasources
Gomplate->>OS: Read environment variables
Gomplate-->>TmplUtils: Return processed templates
TmplUtils->>TmplUtils: Restore original OS env (deferred cleanup)
TmplUtils-->>Config: Return templated result (with env rehydrated)
Config-->>CLI: Final configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📝 WalkthroughWalkthroughThis PR fixes environment variable propagation from Atmos templates into gomplate datasources by preserving environment variables dropped by mapstructure during decoding. It introduces helper functions to extract and restore environment state across template processing, extends case-sensitive key handling for template settings, and updates dependency versions. Changes
Sequence DiagramsequenceDiagram
participant Config as Config Loader
participant Viper as Viper (Case Handling)
participant Templates as Template Processor
participant Mapstructure as Mapstructure Decode
participant OS as OS Environment
participant Gomplate as Gomplate Datasource
Config->>Config: Load raw settings map
Config->>Viper: Apply case-sensitive preservation<br/>for templates.settings.env
Templates->>Templates: Extract env from raw map<br/>(before decode)
Templates->>Mapstructure: Decode settings
Note over Mapstructure: Drops env field
Templates->>Templates: Re-apply extracted env<br/>after decode
Templates->>OS: setEnvVarsWithRestore<br/>(apply env vars)
OS-->>Gomplate: Environment variables available
Gomplate->>Gomplate: Access AWS credentials
Templates->>OS: Cleanup & restore<br/>original env state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
docs/fixes/2026-02-16-gomplate-datasource-env-vars.md (1)
151-155: Add a language identifier to the fenced block.
This quiets MD040 while keeping the rendered output unchanged.Suggested tweak
-``` +```text ok github.com/cloudposse/atmos/internal/exec 1.207s ok github.com/cloudposse/atmos/pkg/config 3.984s ok github.com/cloudposse/atmos/pkg/schema 0.273s</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/fixes/2026-02-16-gomplate-datasource-env-vars.mdaround lines 151 -
155, The fenced code block showing the test output (the triple-backtick block
containing the three "ok github.com/..." lines) lacks a language identifier and
triggers MD040; update that block to use a labeled fence (e.g.,text) so the markdown linter is satisfied while keeping the rendered output identical—modify the existing triple-backtick block in docs/fixes/2026-02-16-gomplate-datasource-env-vars.md totext at the start
and close with ``` at the end.</details> </blockquote></details> <details> <summary>internal/exec/template_utils.go (3)</summary><blockquote> `362-386`: **Duplicated env-type-switch logic with `extractEnvFromRawMap`.** The switch on `envRaw.(type)` at lines 366-375 mirrors the same pattern inside `extractEnvFromRawMap` (lines 121-132). Consider extracting a small `convertRawEnvToStringMap(envRaw any) map[string]string` helper to DRY this up. <details> <summary>Sketch</summary> ```go func convertRawEnvToStringMap(envRaw any) map[string]string { result := make(map[string]string) switch env := envRaw.(type) { case map[string]any: for k, v := range env { if s, ok := v.(string); ok { result[k] = s } } case map[string]string: return env } if len(result) == 0 { return nil } return result } ``` Then both `extractEnvFromRawMap` and the inline block here can delegate to it. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 362 - 386, The inline type-switch that converts envRaw into resultEnv duplicates logic already in extractEnvFromRawMap; create a small helper convertRawEnvToStringMap(envRaw any) map[string]string (or similar) that performs the switch and returns nil on empty, then replace the inline switch around resultMapSettingsTemplatesSettings["env"] with a call to convertRawEnvToStringMap and use its return to populate resultEnv before decoding into templateSettings, and update extractEnvFromRawMap to delegate to the new helper so both sites reuse the same conversion logic. ``` </details> --- `147-177`: **`setEnvVarsWithRestore` — solid save/restore pattern.** Two minor observations: 1. Lines 169 and 171: `os.Setenv` / `os.Unsetenv` errors in the cleanup closure are silently discarded. These rarely fail in practice, but if one does, the env ends up in a partially restored state with no signal. Consider at minimum logging a warning. 2. On error at line 161-163, the function returns `nil` for the cleanup function. Callers must nil-check before calling cleanup, or they'll panic. Currently the single call site (line 290-294) returns early on error so it never calls cleanup — but a future caller could trip on this. Returning a no-op `func(){}` instead of `nil` would be safer. <details> <summary>Suggested defensive return</summary> ```diff for k, v := range envVars { if err := os.Setenv(k, v); err != nil { - return nil, err + // Best-effort rollback of already-set vars. + for rk, orig := range saved { + if orig.existed { + os.Setenv(rk, orig.value) + } else { + os.Unsetenv(rk) + } + } + return func() {}, fmt.Errorf("setting env var %s: %w", k, err) } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 147 - 177, The setEnvVarsWithRestore function currently discards os.Setenv/os.Unsetenv errors inside the cleanup closure and returns nil for the cleanup on a failed initial Setenv; update the cleanup closure in setEnvVarsWithRestore to handle and at minimum log warnings when os.Setenv or os.Unsetenv returns an error (referencing the cleanup closure and saved map/savedEnvVar) so failures during restore are visible, and change the function to always return a non-nil cleanup function (e.g., a no-op func(){} when returning early on an os.Setenv error) instead of nil so callers of setEnvVarsWithRestore can safely call the returned cleanup without a nil-check. ``` </details> --- `288-295`: **`defer cleanup()` inside the evaluation loop accumulates defers.** Each loop iteration pushes a new deferred cleanup. With the typical 1–3 evaluations this is fine — LIFO ordering ensures correct restoration. Just be aware that if `evaluations` ever becomes large, this would stack many defers unnecessarily. If you want to tighten this later, you could call `cleanup()` explicitly at the end of each iteration and only `defer` a final safety net, but the current approach is correct. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 288 - 295, The loop currently calls setEnvVarsWithRestore(templateSettings.Env) and uses defer cleanup() which accumulates deferred calls per iteration; change this to call cleanup() explicitly at the end of each iteration (instead of deferring) to avoid building up many defers—i.e., replace defer cleanup() with a local call to cleanup() just before the iteration continues/exits; if you want a safety net keep a single top-level defer that runs only once outside the loop, but ensure setEnvVarsWithRestore, cleanup(), and the evaluation loop are adjusted so each iteration restores its env immediately. ``` </details> </blockquote></details> <details> <summary>internal/exec/template_utils_env_test.go (2)</summary><blockquote> `176-224`: **Prefer `t.Setenv` over bare `os.Unsetenv` for env var setup in tests.** Lines 178, 229, 335, 416, 467 all use `os.Unsetenv` without registering cleanup with the test framework. If `TEST_GOMPLATE_AWS_PROFILE` (or the others) happened to be set in the ambient environment, the test would silently destroy that value with no restore on panic/failure. A safer pattern: ```go // Ensures the var is tracked by the test framework for restore, // then immediately unset it for the test precondition. t.Setenv("TEST_GOMPLATE_AWS_PROFILE", "") os.Unsetenv("TEST_GOMPLATE_AWS_PROFILE") ``` This applies to all the `os.Unsetenv` calls at the top of `TestProcessTmplWithDatasources_*` functions. Based on learnings: "tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils_env_test.go` around lines 176 - 224, Replace the bare os.Unsetenv call at the top of TestProcessTmplWithDatasources_EnvVarsFromConfig (and the other TestProcessTmplWithDatasources_* tests mentioned) with a t.Setenv-backed pattern so the test framework will restore any preexisting value on teardown; specifically, call t.Setenv("TEST_GOMPLATE_AWS_PROFILE", "") to register the variable with the test and then call os.Unsetenv("TEST_GOMPLATE_AWS_PROFILE") to ensure the test precondition, leaving the rest of the test (e.g., ProcessTmplWithDatasources invocation and assertions) unchanged. ``` </details> --- `136-172`: **`TestSetEnvVarsWithRestore` — good coverage of set-and-restore semantics.** One small note: Line 142 uses `os.Unsetenv("TEST_NEW_VAR")` directly. Consider using the `t.Setenv` + `os.Unsetenv` pattern to ensure the test framework tracks the variable's original state, preventing leakage if the test panics before cleanup runs. That said, this is acceptable per project conventions for test env manipulation. Based on learnings: "In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils_env_test.go` around lines 136 - 172, The test TestSetEnvVarsWithRestore currently calls os.Unsetenv("TEST_NEW_VAR") directly which can bypass the test harness tracking; update the setup to use t.Setenv to register the original state and then call os.Unsetenv (or rely on t.Setenv with empty value) so the testing framework records/restores the environment properly before calling setEnvVarsWithRestore; locate the test function TestSetEnvVarsWithRestore and adjust the setup lines around the existing t.Setenv("TEST_EXISTING_VAR", "original_value") and os.Unsetenv("TEST_NEW_VAR") to use t.Setenv for the new variable (or pair t.Setenv with os.Unsetenv) to avoid leaking env state if the test panics. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@NOTICE:
- Around line 24-27: The NOTICE file was modified manually and is out of sync
with the generator; run the provided script ./scripts/generate-notice.sh to
regenerate NOTICE and commit the generated output instead of editing NOTICE by
hand, ensuring the updated NOTICE matches CI expectations (reference:
./scripts/generate-notice.sh and NOTICE).In
@tests/snapshots/TestCLICommands_describe_component_provenance_advanced.stdout.golden:
- Line 41: The snapshot file was edited manually; revert the manual changes to
the golden (restore from VCS) and regenerate the snapshot by rerunning the test
that owns this snapshot (the TestCLICommands test) with the snapshot
regeneration flag (e.g., run the test command with -regenerate-snapshots as in
the review comment) so the updated output for
describe_component_provenance_advanced is produced automatically; ensure you run
the specific test (TestCLICommands/test_name) to update tests/snapshots/**
rather than editing the file by hand.In
@tests/snapshots/TestCLICommands_describe_component_with_provenance_and_stack.stdout.golden:
- Line 41: The snapshot file
tests/snapshots/TestCLICommands_describe_component_with_provenance_and_stack.stdout.golden
was modified manually; instead regenerate it by re-running the test that
produces this golden (the
TestCLICommands_describe_component_with_provenance_and_stack test) with the
snapshot regeneration flag so the snapshot is updated automatically; run the
test using the project’s test harness (e.g., go test ./tests -run
'TestCLICommands/describ e_component_with_provenance_and_stack'
-regenerate-snapshots or the equivalent command used in CI) and commit the
resulting regenerated snapshot rather than editing the golden file by hand.
Nitpick comments:
In@docs/fixes/2026-02-16-gomplate-datasource-env-vars.md:
- Around line 151-155: The fenced code block showing the test output (the
triple-backtick block containing the three "ok github.com/..." lines) lacks a
language identifier and triggers MD040; update that block to use a labeled fence
(e.g.,text) so the markdown linter is satisfied while keeping the rendered output identical—modify the existing triple-backtick block in docs/fixes/2026-02-16-gomplate-datasource-env-vars.md totext at the start
and close with ``` at the end.In
@internal/exec/template_utils_env_test.go:
- Around line 176-224: Replace the bare os.Unsetenv call at the top of
TestProcessTmplWithDatasources_EnvVarsFromConfig (and the other
TestProcessTmplWithDatasources_* tests mentioned) with a t.Setenv-backed pattern
so the test framework will restore any preexisting value on teardown;
specifically, call t.Setenv("TEST_GOMPLATE_AWS_PROFILE", "") to register the
variable with the test and then call os.Unsetenv("TEST_GOMPLATE_AWS_PROFILE") to
ensure the test precondition, leaving the rest of the test (e.g.,
ProcessTmplWithDatasources invocation and assertions) unchanged.- Around line 136-172: The test TestSetEnvVarsWithRestore currently calls
os.Unsetenv("TEST_NEW_VAR") directly which can bypass the test harness tracking;
update the setup to use t.Setenv to register the original state and then call
os.Unsetenv (or rely on t.Setenv with empty value) so the testing framework
records/restores the environment properly before calling setEnvVarsWithRestore;
locate the test function TestSetEnvVarsWithRestore and adjust the setup lines
around the existing t.Setenv("TEST_EXISTING_VAR", "original_value") and
os.Unsetenv("TEST_NEW_VAR") to use t.Setenv for the new variable (or pair
t.Setenv with os.Unsetenv) to avoid leaking env state if the test panics.In
@internal/exec/template_utils.go:
- Around line 362-386: The inline type-switch that converts envRaw into
resultEnv duplicates logic already in extractEnvFromRawMap; create a small
helper convertRawEnvToStringMap(envRaw any) map[string]string (or similar) that
performs the switch and returns nil on empty, then replace the inline switch
around resultMapSettingsTemplatesSettings["env"] with a call to
convertRawEnvToStringMap and use its return to populate resultEnv before
decoding into templateSettings, and update extractEnvFromRawMap to delegate to
the new helper so both sites reuse the same conversion logic.- Around line 147-177: The setEnvVarsWithRestore function currently discards
os.Setenv/os.Unsetenv errors inside the cleanup closure and returns nil for the
cleanup on a failed initial Setenv; update the cleanup closure in
setEnvVarsWithRestore to handle and at minimum log warnings when os.Setenv or
os.Unsetenv returns an error (referencing the cleanup closure and saved
map/savedEnvVar) so failures during restore are visible, and change the function
to always return a non-nil cleanup function (e.g., a no-op func(){} when
returning early on an os.Setenv error) instead of nil so callers of
setEnvVarsWithRestore can safely call the returned cleanup without a nil-check.- Around line 288-295: The loop currently calls
setEnvVarsWithRestore(templateSettings.Env) and uses defer cleanup() which
accumulates deferred calls per iteration; change this to call cleanup()
explicitly at the end of each iteration (instead of deferring) to avoid building
up many defers—i.e., replace defer cleanup() with a local call to cleanup() just
before the iteration continues/exits; if you want a safety net keep a single
top-level defer that runs only once outside the loop, but ensure
setEnvVarsWithRestore, cleanup(), and the evaluation loop are adjusted so each
iteration restores its env immediately.</details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
tests/snapshots/TestCLICommands_describe_component_provenance_advanced.stdout.golden
Show resolved
Hide resolved
tests/snapshots/TestCLICommands_describe_component_with_provenance_and_stack.stdout.golden
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2084 +/- ##
==========================================
+ Coverage 76.19% 76.22% +0.03%
==========================================
Files 824 824
Lines 77546 77622 +76
==========================================
+ Hits 59085 59167 +82
+ Misses 14760 14754 -6
Partials 3701 3701
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
osterman
reviewed
Feb 17, 2026
Add integration tests exercising the full ExecuteDescribeStacks and InitCliConfig paths with templates.settings.env configured. This covers the env extraction at describe_stacks.go caller sites, the load.go case restoration, and verifies case-sensitive key preservation through Viper. Uses filepath.Join for cross-platform path handling in test fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/schema/schema_test.go (1)
184-221: Prefer a table-driven structure for the new scenarios.These two subtests are better expressed as a single table-driven case list for consistency.
As per coding guidelines: “Use table-driven tests for testing multiple scenarios in Go”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/schema_test.go` around lines 184 - 221, Combine the two t.Run blocks into a single table-driven test that iterates over cases (with fields like name, CaseMaps cm, AtmosConfiguration templates.Settings.Env input, and expected map) and within the loop call t.Run(tc.name, func(t *testing.T) { result := atmosConfig.GetCaseSensitiveMap("templates.settings.env"); assert.Equal(t, tc.expected, result) }); locate where AtmosConfiguration, Templates, TemplatesSettings, GetCaseSensitiveMap and casemap.New are used in pkg/schema/schema_test.go and replace the two explicit subtests with this table-driven structure to cover both the no-CaseMaps and case-restoration scenarios.internal/exec/template_utils.go (2)
362-386: Inline env extraction duplicatesextractEnvFromRawMaplogic.The type-switch extraction here (lines 364-376) mirrors
extractEnvFromRawMapbut operates on a slightly different input shape. Consider extracting a shared helper that takes the rawenvvalue directly, to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 362 - 386, The env extraction in the block that inspects resultMapSettingsTemplatesSettings duplicates logic in extractEnvFromRawMap; refactor by adding or reusing a helper that accepts the raw env value (the envRaw extracted from resultMapSettingsTemplatesSettings) and returns a map[string]string, then replace the type-switch with a call to that helper, assign the returned map to resultEnv, and preserve the existing mapstructure.Decode(templateSettings) and the subsequent restore to templateSettings.Env so behavior remains identical.
288-295:defer cleanup()inside the evaluation loop accumulates deferred calls.Each loop iteration appends a new deferred cleanup. With
evaluations > 1, this creates N cleanup functions when only 1 is needed. It works correctly due to LIFO ordering (the first cleanup, which saved the true originals, runs last), but it's wasteful.Consider moving the env setup before the loop, or guarding with a
sync.Once-style flag so it's only set once.♻️ Suggested: move env setup before the loop
+ // Set environment variables for template processing and restore originals when done. + if len(templateSettings.Env) > 0 { + cleanup, envErr := setEnvVarsWithRestore(templateSettings.Env) + if envErr != nil { + return "", envErr + } + defer cleanup() + } + for i := 0; i < evaluations; i++ { log.Trace("ProcessTmplWithDatasources", logKeyTemplate, tmplName, "evaluation", i+1) // ... - // Set environment variables for template processing and restore originals when done. - if len(templateSettings.Env) > 0 { - cleanup, envErr := setEnvVarsWithRestore(templateSettings.Env) - if envErr != nil { - return "", envErr - } - defer cleanup() - }Note: if
templateSettings.Envcan change mid-loop (via the re-decode at lines 362-386), you'd need to callsetEnvVarsWithRestoreagain inside the loop only when env actually changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 288 - 295, The code calls setEnvVarsWithRestore(templateSettings.Env) with defer cleanup() inside the evaluation loop, which accumulates deferred cleanups; move the env setup so it's executed only once per call: invoke setEnvVarsWithRestore before entering the evaluations loop (or protect it with a sync.Once/flag) and defer the single cleanup after that setup; if templateSettings.Env may be re-decoded/changed later in the loop, detect the change and call setEnvVarsWithRestore/cleanup only when Env differs (replacing the previous env and running its cleanup before setting the new one). Ensure you update references to cleanup, setEnvVarsWithRestore, and templateSettings.Env accordingly.internal/exec/template_utils_env_test.go (1)
176-224:os.Unsetenvcalls in ProcessTmplWithDatasources tests.Lines 178, 229, 335, 416, 467 all use
os.Unsetenvfor setup. Same recommendation as the integration test file — considert.Setenv("VAR", "")+os.Unsetenv("VAR")for automatic restore on panic. Low risk since these are test-only vars.Based on learnings: "In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils_env_test.go` around lines 176 - 224, Replace the manual os.Unsetenv setup/teardown in TestProcessTmplWithDatasources_EnvVarsFromConfig (and the other ProcessTmplWithDatasources tests referenced) with testing.T's scoped helper to ensure automatic restore: call t.Setenv("TEST_GOMPLATE_AWS_PROFILE", "") at the start to ensure the var is empty for the test and remove the manual os.Unsetenv cleanup; keep using ProcessTmplWithDatasources as before but rely on t.Setenv for isolation and remove/assert adjustments that expect manual restoration.internal/exec/template_env_integration_test.go (1)
21-22: Prefert.Setenvfor env cleanup safety.Lines 21-22 use
os.Unsetenvdirectly. If these vars happen to exist in the parent env and the test panics, they won't be restored. Usingt.Setenv("TEST_TMPL_ENV_VAR", "")followed byos.Unsetenv(...)ensures automatic restore on test cleanup.That said, since these are test-specific vars unlikely to exist, the practical risk is low.
Based on learnings: "In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_env_integration_test.go` around lines 21 - 22, Replace direct calls to os.Unsetenv("TEST_TMPL_ENV_VAR") and os.Unsetenv("TEST_TMPL_STACK_VAR") in the test with testing.T's Setenv so the original environment is restored on test cleanup; locate the test function in internal/exec/template_env_integration_test.go (where t is the *testing.T used) and call t.Setenv("TEST_TMPL_ENV_VAR", "") and t.Setenv("TEST_TMPL_STACK_VAR", "") instead of Unsetenv to ensure safe setup/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-02-16-gomplate-datasource-env-vars.md`:
- Around line 151-155: The fenced code block currently has no language tag (it
starts with ``` and contains the "ok github.com/cloudposse/..." lines); update
the fence to include a language such as text or console by changing the opening
``` to ```text (or ```console) so the block becomes ```text followed by the
three "ok ..." lines and the closing ``` to satisfy markdownlint.
---
Duplicate comments:
In `@NOTICE`:
- Around line 24-27: The NOTICE file was modified by hand; revert manual edits
and regenerate NOTICE using the repository's NOTICE generator script so
CI/license tracking remains consistent; locate the NOTICE file and run the
project's NOTICE generation tool (the generator script used in this repo) to
produce the correct content rather than editing NOTICE directly.
In
`@tests/snapshots/TestCLICommands_describe_component_provenance_advanced.stdout.golden`:
- Line 41: The snapshot was manually edited; regenerate it instead of touching
the golden file: run the test case
TestCLICommands/describe_component_provenance_advanced with the
-regenerate-snapshots flag (e.g. go test ./tests -run
'TestCLICommands/describe_component_provenance_advanced' -regenerate-snapshots)
to recreate
tests/snapshots/TestCLICommands_describe_component_provenance_advanced.stdout.golden
and revert your manual change to restore authoritative snapshot handling.
In
`@tests/snapshots/TestCLICommands_describe_component_with_provenance_and_stack.stdout.golden`:
- Line 41: The snapshot was edited manually; instead regenerate it by running
the test suite for the failing test
(TestCLICommands_describe_component_with_provenance_and_stack) with the
-regenerate-snapshots flag so the golden file is updated automatically (e.g.,
run the go test command shown in the review with -regenerate-snapshots) and
commit the regenerated snapshot instead of making manual edits.
---
Nitpick comments:
In `@internal/exec/template_env_integration_test.go`:
- Around line 21-22: Replace direct calls to os.Unsetenv("TEST_TMPL_ENV_VAR")
and os.Unsetenv("TEST_TMPL_STACK_VAR") in the test with testing.T's Setenv so
the original environment is restored on test cleanup; locate the test function
in internal/exec/template_env_integration_test.go (where t is the *testing.T
used) and call t.Setenv("TEST_TMPL_ENV_VAR", "") and
t.Setenv("TEST_TMPL_STACK_VAR", "") instead of Unsetenv to ensure safe
setup/teardown.
In `@internal/exec/template_utils_env_test.go`:
- Around line 176-224: Replace the manual os.Unsetenv setup/teardown in
TestProcessTmplWithDatasources_EnvVarsFromConfig (and the other
ProcessTmplWithDatasources tests referenced) with testing.T's scoped helper to
ensure automatic restore: call t.Setenv("TEST_GOMPLATE_AWS_PROFILE", "") at the
start to ensure the var is empty for the test and remove the manual os.Unsetenv
cleanup; keep using ProcessTmplWithDatasources as before but rely on t.Setenv
for isolation and remove/assert adjustments that expect manual restoration.
In `@internal/exec/template_utils.go`:
- Around line 362-386: The env extraction in the block that inspects
resultMapSettingsTemplatesSettings duplicates logic in extractEnvFromRawMap;
refactor by adding or reusing a helper that accepts the raw env value (the
envRaw extracted from resultMapSettingsTemplatesSettings) and returns a
map[string]string, then replace the type-switch with a call to that helper,
assign the returned map to resultEnv, and preserve the existing
mapstructure.Decode(templateSettings) and the subsequent restore to
templateSettings.Env so behavior remains identical.
- Around line 288-295: The code calls
setEnvVarsWithRestore(templateSettings.Env) with defer cleanup() inside the
evaluation loop, which accumulates deferred cleanups; move the env setup so it's
executed only once per call: invoke setEnvVarsWithRestore before entering the
evaluations loop (or protect it with a sync.Once/flag) and defer the single
cleanup after that setup; if templateSettings.Env may be re-decoded/changed
later in the loop, detect the change and call setEnvVarsWithRestore/cleanup only
when Env differs (replacing the previous env and running its cleanup before
setting the new one). Ensure you update references to cleanup,
setEnvVarsWithRestore, and templateSettings.Env accordingly.
In `@pkg/schema/schema_test.go`:
- Around line 184-221: Combine the two t.Run blocks into a single table-driven
test that iterates over cases (with fields like name, CaseMaps cm,
AtmosConfiguration templates.Settings.Env input, and expected map) and within
the loop call t.Run(tc.name, func(t *testing.T) { result :=
atmosConfig.GetCaseSensitiveMap("templates.settings.env"); assert.Equal(t,
tc.expected, result) }); locate where AtmosConfiguration, Templates,
TemplatesSettings, GetCaseSensitiveMap and casemap.New are used in
pkg/schema/schema_test.go and replace the two explicit subtests with this
table-driven structure to cover both the no-CaseMaps and case-restoration
scenarios.
) - Convert schema_test.go subtests to table-driven structure - Extract convertRawEnvToStringMap shared helper to reduce duplication - Move env setup before evaluation loop to avoid accumulating deferred cleanups - Use t.Setenv for automatic restore on test cleanup in all env tests - Add language identifier to fenced code block in fix doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
osterman
approved these changes
Feb 17, 2026
osterman
previously approved these changes
Feb 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/exec/template_utils_env_test.go (2)
416-511: Evaluation loop and case-sensitivity tests round out the coverage nicely. The eval loop test withEvaluations: 2and embeddedsettings.templates.settings.envin the template output validates the re-extraction path in the inner loop. The case-sensitivity test directly targets Bug 2 (Viper lowercasing).One note: many inline comments across this file lack trailing periods (e.g., lines 138, 141, 152, 156, etc.). The coding guidelines enforce
godot— ending comments with periods. Low priority for a test file, but worth a pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils_env_test.go` around lines 416 - 511, Update inline comments in internal/exec/template_utils_env_test.go to follow the godot rule by ensuring every sentence-style comment ends with a trailing period; specifically audit comments inside and around the TestProcessTmplWithDatasources_EnvVarsInEvaluationLoop and TestProcessTmplWithDatasources_EnvVarsCaseSensitive tests (and other inline comments earlier in the file) and add a period to any that are missing to satisfy the coding guidelines.
136-172: Solid coverage of the set-and-restore cycle. Thet.Setenv+os.Unsetenvpattern correctly registers cleanup while starting from a clean state. One minor note:TEST_NEW_VARon line 142 isn't registered witht.Setenv, so if the test panics beforecleanup()runs, it could leak. Consider addingt.Setenvfor it too.Suggested safeguard
- // Ensure a var that doesn't exist. - os.Unsetenv("TEST_NEW_VAR") + // Ensure a var that doesn't exist (register for cleanup first). + t.Setenv("TEST_NEW_VAR", "") + os.Unsetenv("TEST_NEW_VAR")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils_env_test.go` around lines 136 - 172, The test may leak an environment variable if it panics before cleanup because TEST_NEW_VAR isn't registered with t.Setenv; to fix, register TEST_NEW_VAR with t.Setenv before removing it so the testing framework will restore previous state on test exit (i.e., call t.Setenv("TEST_NEW_VAR", "") prior to os.Unsetenv("TEST_NEW_VAR")), keeping the rest of TestSetEnvVarsWithRestore and the setEnvVarsWithRestore usage unchanged.internal/exec/template_utils.go (1)
160-188: Partial env set on error leaves vars unrestored.If
os.Setenvfails partway through the second loop (line 172), previously-set vars from earlier iterations won't be rolled back sincenilis returned as the cleanup function.Practically,
os.Setenvrarely fails (empty key on Windows), so this is low risk. Still, returning the cleanup for already-set vars would be more robust.Suggested hardening
for k, v := range envVars { if err := os.Setenv(k, v); err != nil { - return nil, err + // Return cleanup for vars already set before the failure. + cleanup := func() { + for key, original := range saved { + if original.existed { + os.Setenv(key, original.value) + } else { + os.Unsetenv(key) + } + } + } + return cleanup, err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/template_utils.go` around lines 160 - 188, The function setEnvVarsWithRestore currently returns nil cleanup if os.Setenv fails partway, leaving earlier-set vars unrestored; modify it so a cleanup function is always returned even on error: either construct the cleanup before the second loop (using the saved map) or, upon encountering an os.Setenv error inside the loop in setEnvVarsWithRestore, immediately build and return a cleanup closure that restores/unsets only the vars that were set up to that point along with the error; ensure references to the saved map and the keys set so far (the loop variables in setEnvVarsWithRestore) are used to perform the rollback and return (cleanup, err) instead of (nil, err).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-02-16-gomplate-datasource-env-vars.md`:
- Line 98: The function signature for setEnvVarsWithRestore has a spacing typo
in its return type; change the declared return from "func ()" to "func()" so the
signature reads setEnvVarsWithRestore(envVars map[string]string) (func(), error)
— update the declaration of setEnvVarsWithRestore accordingly.
- Around line 103-108: Add a trailing period to the last bullet in section 3 of
the docs so the sentence ending "Also extracts env from the re-decoded template
settings within the evaluation loop" becomes a complete sentence; locate the
bullet that references templateSettings.Env, atmosConfig.Templates.Settings.Env,
settingsSection.Templates.Settings.Env and setEnvVarsWithRestore and append a
period to the end.
In `@internal/exec/template_utils.go`:
- Around line 374-389: The evaluation loop currently os.Setenv()'s keys from
resultEnv (via convertRawEnvToStringMap) but those new keys aren't included in
the deferred cleanup from setEnvVarsWithRestore/cleanup(), causing an env leak;
modify the code around templateSettings.Env assignment (the block that decodes
into templateSettings and then sets templateSettings.Env and os.Setenv) to track
which keys in resultEnv were newly introduced (not present in the original
templateSettings.Env or the restore snapshot returned by setEnvVarsWithRestore),
and extend the cleanup behavior to either restore previous values for
overwritten keys and remove keys that were newly added; use the same restoration
mechanism used by setEnvVarsWithRestore (or return an augmented cleanup closure)
so the deferred cleanup removes any extra keys introduced in the loop and
restores overwritten values.
---
Nitpick comments:
In `@internal/exec/template_utils_env_test.go`:
- Around line 416-511: Update inline comments in
internal/exec/template_utils_env_test.go to follow the godot rule by ensuring
every sentence-style comment ends with a trailing period; specifically audit
comments inside and around the
TestProcessTmplWithDatasources_EnvVarsInEvaluationLoop and
TestProcessTmplWithDatasources_EnvVarsCaseSensitive tests (and other inline
comments earlier in the file) and add a period to any that are missing to
satisfy the coding guidelines.
- Around line 136-172: The test may leak an environment variable if it panics
before cleanup because TEST_NEW_VAR isn't registered with t.Setenv; to fix,
register TEST_NEW_VAR with t.Setenv before removing it so the testing framework
will restore previous state on test exit (i.e., call t.Setenv("TEST_NEW_VAR",
"") prior to os.Unsetenv("TEST_NEW_VAR")), keeping the rest of
TestSetEnvVarsWithRestore and the setEnvVarsWithRestore usage unchanged.
In `@internal/exec/template_utils.go`:
- Around line 160-188: The function setEnvVarsWithRestore currently returns nil
cleanup if os.Setenv fails partway, leaving earlier-set vars unrestored; modify
it so a cleanup function is always returned even on error: either construct the
cleanup before the second loop (using the saved map) or, upon encountering an
os.Setenv error inside the loop in setEnvVarsWithRestore, immediately build and
return a cleanup closure that restores/unsets only the vars that were set up to
that point along with the error; ensure references to the saved map and the keys
set so far (the loop variables in setEnvVarsWithRestore) are used to perform the
rollback and return (cleanup, err) instead of (nil, err).
- Return cleanup function on partial os.Setenv failure in setEnvVarsWithRestore - Track and clean up extra env keys introduced by evaluation loop - Register TEST_NEW_VAR with t.Setenv for panic safety - Fix typos in fix doc (func() spacing, trailing period) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
These changes were released in v1.206.3-rc.1. |
|
These changes were released in v1.207.0. |
|
These changes were released in v1.208.0-rc.0. |
|
These changes were released in v1.208.0-test.15. |
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
2 participants
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.
what
templates.settings.envfrom propagating environment variables to gomplate datasourcesmapstructure:"-"tag onTemplatesSettings.Envsilently dropped the env field during the encode/decode/merge pipeline inProcessTmplWithDatasourcesAWS_PROFILE→aws_profile) andcaseSensitivePathsdidn't includetemplates.settings.envutils.goanddescribe_stacks.gogocloud.dev,terraform-exec) after go.mod updatewhy
templates.settings.envwith AWS credentials (e.g.,AWS_PROFILE,AWS_REGION) expected gomplate datasources (S3, SSM, Secrets Manager) to use those credentials, but env vars were never set in the OS environmentos.Setenvloop inProcessTmplWithDatasourcesnever executed becausetemplateSettings.Envwas always nil after mapstructure dropped itreferences
docs/fixes/2026-02-16-gomplate-datasource-env-vars.mdSummary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores