Repository navigation
fix(jit): honor metadata.component subpath for JIT source-provisioned components - #2371
Merged
Andriy Knysh (aknysh) merged 21 commits intoMay 4, 2026
Conversation
When a JIT source component sets metadata.component to a subdirectory within the cloned repo (e.g. modules/iam-policy), provisionComponentSource was returning the repo root from WorkdirPathKey verbatim, ignoring the subpath. All generated files (backend.tf.json, varfile) and the tofu working directory landed in the repo root instead of the module subdir. Add applyMetadataComponentSubpath helper that joins BaseComponentPath onto the workdir root, then store the corrected path back into WorkdirPathKey so all 13+ downstream consumers (varfile, planfile, backend, init, shell, packer, helmfile, ansible) fix themselves automatically. Fixes cloudposse#2364.
…oudposse#2364) Add null-label-exports component to source-provisioner-workdir fixture: uses full terraform-null-label repo (no //subpath in URI) plus metadata.component: exports. Integration test verifies the full repo is provisioned and the exports/ module subdir is accessible after the fix.
… fix - Extract applyMetadataComponentSubpath + provisionComponentSource to terraform_provision_helpers.go (keeps terraform_execute_helpers.go under 600-line limit) - Change applyMetadataComponentSubpath signature to (baseComponentPath, workdirPath string) — only uses one field from ConfigAndStacksInfo - Add WorkdirSubpathAppliedKey sentinel to prevent double-joining if provisionComponentSource is called twice on the same ComponentSection - Propagate u.IsDirectory error for non-ErrNotExist failures (permission errors, broken mounts) instead of silently discarding them - Fix terraform_verify_plan.go and terraform_plan_diff.go: both called ProcessStacks which builds a fresh ComponentSection with no WorkdirPathKey, making the old workdir check dead code for JIT components; now use workdir.BuildPath + applyMetadataComponentSubpath to compute the correct path from first principles - Update unit tests for new function signature; fix DotDotEscapeHatch test to assert the result actually escapes the workdir boundary
- Wrap "workdir not accessible" error with errUtils.ErrWorkdirProvision static sentinel so it composes with the project's error policy. - Rename applyMetadataComponentSubpath param baseComponentPath -> metadataComponentSubpath; the value is the metadata.component subpath, not a filesystem base path. - Expand godoc on applyMetadataComponentSubpath to spell out why ".." traversal is intentional (sibling-module repos rely on relative parent paths) and link the threat-model rationale to atmos's trusted-operator model. - Add network/git preconditions to TestJITSource_MetadataComponentSubpath so it skips gracefully on offline runners instead of hard-failing.
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes JIT workdir provisioning and metadata.component subpath resolution in a new pkg/component workdir helper; executors (Terraform, Helmfile, Packer, Ansible) now call these helpers to provision/resolve component paths. Adds unit tests for the helpers and regression tests validating metadata.component behavior for workdir-provisioned components. ChangesWorkdir-Aware Component Path Resolution & Executor Integration
Sequence Diagram(s)sequenceDiagram
participant Exec as Executor
participant Component as pkg/component
participant Prov as Provisioner
participant FS as Filesystem
Exec->>Component: ProvisionAndResolveComponentPath(ctx, config, info, type, fallback)
Component->>Prov: AutoProvisionSource (if source present)
Prov-->>Component: provision result / error
Component->>Component: ApplyWorkdirSubpathToSection(info) (resolve metadata.component)
Component->>FS: stat(resolvedPath)
FS-->>Component: exists / error
Component-->>Exec: resolvedPath, exists, err
Exec->>FS: generate files / run tool in resolvedPath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (1)
tests/cli_source_provisioner_workdir_test.go (1)
132-149: Strengthen this regression assertion to validate the effective execution path.These checks only prove the cloned repo contains
exports/exports.tf, which was true before the fix as well. Add one assertion that fails on pre-fix behavior (for example, assert a Terraform-generated artifact or resolved run path is under<workdir>/exports, not just thatexports/exists).As per coding guidelines "Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use
errors.Is()for error checking."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_source_provisioner_workdir_test.go` around lines 132 - 149, Add an assertion that verifies Terraform actually ran under the cloned module directory by checking for a Terraform-generated artifact inside exportsDir (e.g., assert that filepath.Join(exportsDir, ".terraform.lock.hcl") or the ".terraform" directory exists), instead of only asserting exports/ exists; update the test around the existing exportsDir/exportsTfPath checks to stat the lockfile or .terraform directory (use exportsDir variable) and require.NoError/require.True accordingly so the assertion fails on pre-fix behavior where Terraform was not executed in the exports subdir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_plan_diff.go`:
- Around line 78-80: The current os.Stat(candidate) call silently ignores errors
other than a successful stat; change the logic in the function that sets
componentPath (the block using os.Stat(candidate), candidate and componentPath)
so that if os.Stat returns an error you check os.IsNotExist(err) and only use
the fallback when the file truly does not exist, but for any other error
(permission, I/O, etc.) return that error up the call stack; mirror the same
guard/behavior you already use in VerifyPlanfile to keep error handling
consistent.
In `@internal/exec/terraform_provision_helpers.go`:
- Around line 61-63: The current return uses fmt.Errorf directly when
provSource.AutoProvisionSource fails; replace that with a wrapped sentinel from
errors/errors.go so upstream can classify the failure. Change the error return
to join the static sentinel (e.g., errors.ErrAutoProvisionSource) with a
contextual fmt.Errorf wrapping autoErr using %w, for example: return "", false,
errors.Join(errors.ErrAutoProvisionSource, fmt.Errorf("failed to auto-provision
component source: %w", autoErr)); ensure you import the project errors package
and reference the provSource.AutoProvisionSource call and
info.ComponentSection/info.AuthContext context as before.
---
Nitpick comments:
In `@tests/cli_source_provisioner_workdir_test.go`:
- Around line 132-149: Add an assertion that verifies Terraform actually ran
under the cloned module directory by checking for a Terraform-generated artifact
inside exportsDir (e.g., assert that filepath.Join(exportsDir,
".terraform.lock.hcl") or the ".terraform" directory exists), instead of only
asserting exports/ exists; update the test around the existing
exportsDir/exportsTfPath checks to stat the lockfile or .terraform directory
(use exportsDir variable) and require.NoError/require.True accordingly so the
assertion fails on pre-fix behavior where Terraform was not executed in the
exports subdir.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ceda2a7b-3166-4507-97df-a46497468fa5
📒 Files selected for processing (9)
internal/exec/terraform_execute_helpers.gointernal/exec/terraform_execute_helpers_test.gointernal/exec/terraform_plan_diff.gointernal/exec/terraform_provision_helpers.gointernal/exec/terraform_verify_plan.gopkg/provisioner/workdir/types.gotests/cli_source_provisioner_workdir_test.gotests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/source-with-workdir-metadata-component.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
💤 Files with no reviewable changes (1)
- internal/exec/terraform_execute_helpers.go
- Surface non-ENOENT stat failures in plan-diff/verify-plan instead of silently falling back to the local component path. EACCES, EIO, broken symlinks etc. now wrap ErrWorkdirProvision with a path-prefixed message, while ENOENT still falls back gracefully so a not-yet-provisioned workdir surfaces as the existing "missing planfile" error rather than a new one. - Wrap the AutoProvisionSource error with errUtils.ErrWorkdirProvision so upstream callers can classify provisioning failures via errors.Is. - Extract resolveWorkdirComponentPath helper to dedupe the workdir-path derivation block that was duplicated between TerraformPlanDiff and VerifyPlanfile. Both call sites now share one implementation, so a future change to BuildPath semantics propagates everywhere automatically. - Extract applyWorkdirSubpathToSection from provisionComponentSource so the WorkdirPathKey mutation is unit-testable without standing up a real source provisioner. The integration test in tests/cli_source_provisioner_workdir_test.go asserts that the cloned repo is reachable; the new unit tests assert the load-bearing behavior — that WorkdirPathKey is rewritten to the joined subpath and that the WorkdirSubpathAppliedKey sentinel prevents a second invocation from joining twice. - Expand the WorkdirSubpathAppliedKey doc comment to spell out the relationship with the source provisioner's invocationDoneKey: the second call short-circuits the provisioner but still observes the already-joined WorkdirPathKey, which is exactly the case the sentinel exists to handle. - Note in applyMetadataComponentSubpath's godoc that absolute paths are rooted under workdirPath per filepath.Join semantics — callers should not rely on absolute paths to escape the workdir.
Contributor
Author
|
Re: the regression-assertion nitpick on The tests assert the load-bearing behavior the integration test could not, without needing a real
These tests would all fail if the fix were reverted, which the integration test alone would not. |
- Drop TestApplyWorkdirSubpathToSection_EmptyBasePath. Its assertions duplicated TestApplyWorkdirSubpathToSection_JoinsSubpath and the godoc claim about the sentinel was never actually asserted. - Drop TestApplyWorkdirSubpathToSection_MissingKey. Documenting the early-return contract via a single-branch test was incomplete (only the absent-key case, not the empty-string case) and the function's behavior there is obvious from two lines of code. - Drop the file-header comment on terraform_provision_helpers.go that explained what the file contains rather than why; CLAUDE.md says don't. - Rewrite the godoc on TestJITSource_MetadataComponentSubpath to be honest about what it tests (smoke test for the provisioning pipeline) rather than claiming to be a regression test for cloudposse#2364 — the actual regression guard lives in the applyWorkdirSubpathToSection unit tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/terraform_execute_helpers_test.go (1)
750-750: Prefer using theprovWorkdir.WorkdirPathconstant over the.workdirliteral.Using the constant keeps the test in sync if that value is ever renamed, and aligns with the pattern already established throughout the codebase.
- expectedRoot := filepath.Join(basePath, ".workdir", cfg.TerraformComponentType, stack+"-"+componentName) + expectedRoot := filepath.Join(basePath, provWorkdir.WorkdirPath, cfg.TerraformComponentType, stack+"-"+componentName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_execute_helpers_test.go` at line 750, The test builds expectedRoot using a hardcoded ".workdir" literal; replace that literal with the provWorkdir.WorkdirPath constant so the test follows the shared convention and remains correct if the workdir name changes — update the filepath.Join call that constructs expectedRoot (using basePath, provWorkdir.WorkdirPath, cfg.TerraformComponentType, stack+"-"+componentName) and add the provWorkdir package import if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_execute_helpers_test.go`:
- Around line 790-818: The test
TestResolveWorkdirComponentPath_StatErrorPropagates relies on POSIX permission
behavior and should be skipped on Windows: add a runtime.GOOS == "windows" guard
at the start of TestResolveWorkdirComponentPath_StatErrorPropagates alongside
the existing os.Getuid() check so the test is skipped on Windows, and add
"runtime" to the import block; no other test logic changes are needed (this
preserves the Linux/macOS EACCES path exercised by resolveWorkdirComponentPath).
---
Nitpick comments:
In `@internal/exec/terraform_execute_helpers_test.go`:
- Line 750: The test builds expectedRoot using a hardcoded ".workdir" literal;
replace that literal with the provWorkdir.WorkdirPath constant so the test
follows the shared convention and remains correct if the workdir name changes —
update the filepath.Join call that constructs expectedRoot (using basePath,
provWorkdir.WorkdirPath, cfg.TerraformComponentType, stack+"-"+componentName)
and add the provWorkdir package import if missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 276cb151-c653-415d-91e2-a2423352fa47
📒 Files selected for processing (5)
internal/exec/terraform_execute_helpers_test.gointernal/exec/terraform_plan_diff.gointernal/exec/terraform_provision_helpers.gointernal/exec/terraform_verify_plan.gopkg/provisioner/workdir/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/provisioner/workdir/types.go
- Skip TestResolveWorkdirComponentPath_StatErrorPropagates on Windows. os.Chmod doesn't deny directory traversal there, and os.Getuid returns -1 not 0 so the existing root-skip never triggers — the test would always fail on Windows CI. - Use provWorkdir.WorkdirPath instead of the ".workdir" literal in TestResolveWorkdirComponentPath_ExistingDir so the test follows the shared constant convention.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/exec/terraform_provision_helpers.go (3)
102-130: LGTM — sentinel wrapping, idempotent join, and ENOENT handling all line up.
errors.Join(errUtils.ErrWorkdirProvision, …)matches the prior review feedback, the post-provision branch correctly delegates toapplyWorkdirSubpathToSectionso the sentinel guards against double-joining on init→plan, and the!errors.Is(errDir, os.ErrNotExist)guard keeps "not provisioned yet" out of the error path. Nice tight pipeline.One minor thought: the 5-minute timeout on line 113 is hardcoded. Not a blocker, but if anyone hits a slow clone in CI, a constant near the top of the file (or a config knob) would make it easier to tune later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_provision_helpers.go` around lines 102 - 130, The 5-minute hardcoded timeout in provisionComponentSource (ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)) should be extracted to a named constant or configurable variable so it can be tuned (e.g., defaultProvisionTimeout or provisionTimeout) instead of being magic in-place; change the ctx creation to use that constant (and update any related docs/comments) and consider exposing it as a package-level var or config knob used by provSource.AutoProvisionSource calls for easier CI tuning.
79-97: Non-directory atcandidateis silently treated as "doesn't exist".When
os.Statsucceeds but the path is a regular file (or symlink to one), line 96 returnsexists=false, err=nil. Callers interraform_plan_diff.go/terraform_verify_plan.gowill then quietly fall back tocomponentPathwithout any signal that a non-directory file is squatting at the expected workdir location. That's almost certainly a misconfiguration worth surfacing.♻️ Suggested tweak
fi, err := os.Stat(candidate) if err != nil { if errors.Is(err, os.ErrNotExist) { return candidate, false, nil } return "", false, errors.Join(errUtils.ErrWorkdirProvision, fmt.Errorf("stat workdir component path %q: %w", candidate, err)) } - return candidate, fi.IsDir(), nil + if !fi.IsDir() { + return "", false, errors.Join(errUtils.ErrWorkdirProvision, fmt.Errorf("workdir component path %q exists but is not a directory", candidate)) + } + return candidate, true, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_provision_helpers.go` around lines 79 - 97, The function resolveWorkdirComponentPath currently treats a non-directory file at the candidate path as "doesn't exist"; change it to surface an error instead: after os.Stat succeeds in resolveWorkdirComponentPath, check fi.IsDir() and if false return the candidate path along with an explicit error (wrap with errUtils.ErrWorkdirProvision and a message like "workdir component path is not a directory") so callers like terraform_plan_diff.go and terraform_verify_plan.go can fail fast and report misconfiguration rather than silently falling back to componentPath.
57-68: Code is correct; optional clarity improvement.
BaseComponentPathcorrectly holds themetadata.componentvalue from YAML config (assigned viaProcessComponentMetadata), not a filesystem path. The function doc at lines 19–34 explicitly discussesmetadata.component, so the join at line 63 is sound.Adding a one-line comment explaining why
BaseComponentPathis the right source (e.g.,// BaseComponentPath holds metadata.component from config inheritance) would help future readers avoid the same double-take.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_provision_helpers.go` around lines 57 - 68, In applyWorkdirSubpathToSection, add a single-line clarifying comment explaining that info.BaseComponentPath contains the metadata.component value from the YAML config (set by ProcessComponentMetadata) so readers understand why applyMetadataComponentSubpath(info.BaseComponentPath, workdirPath) is correct; place the comment immediately above the call to applyMetadataComponentSubpath (or above the function body near the use of info.BaseComponentPath) and reference the symbols applyWorkdirSubpathToSection, info.BaseComponentPath, applyMetadataComponentSubpath, and provWorkdir.WorkdirPathKey in the note.tests/cli_source_provisioner_workdir_test.go (1)
114-154: Smoke test reads cleanly — one note on external-repo coupling.The setup is solid:
RequireExecutable+RequireGitHubAccessskip-gates,--dry-runto avoid needingtofu,t.Cleanupfor.workdir, and a doc comment that's honest about which assertions are the actual regression guard (the unit tests). Pinning toterraform-null-label@0.25.0is the right call.The remaining brittleness is that the test silently passes if
cmd.Execute()short-circuits before provisioning ever runs (e.g., a future arg-parsing change that rejects--dry-runearly). The threeos.Statchecks would still fire, but the failure message would point at "workdir not created" rather than at the real cause. A cheap mitigation: capture the error from line 134 and, if provisioning didn't happen, include the error string in therequire.NoErrorfailure message on line 139.♻️ Optional tweak
- _ = cmd.Execute() // error expected (no terraform binary needed); provisioning still runs + execErr := cmd.Execute() // error expected (no terraform binary needed); provisioning still runs // Verify the workdir root was created — the full repo was cloned here. workdirRoot := filepath.Join(".workdir", "terraform", "dev-null-label-exports") info, statErr := os.Stat(workdirRoot) - require.NoError(t, statErr, "workdir root should exist at %s after provisioning", workdirRoot) + require.NoError(t, statErr, "workdir root should exist at %s after provisioning (cmd.Execute err: %v)", workdirRoot, execErr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_source_provisioner_workdir_test.go` around lines 114 - 154, The test currently discards the error returned by cmd.Execute(), which can hide that provisioning never ran; change the code around the cmd.Execute() call to capture its error (e.g., execErr := cmd.Execute()) and then, when asserting the workdir root exists (the require.NoError checking statErr for workdirRoot), include execErr in the failure message so the test output shows the Execute error if provisioning didn't run; reference cmd.Execute() and the workdirRoot/statErr(require.NoError) checks to locate where to capture and propagate the execution error into the require.NoError failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/exec/terraform_provision_helpers.go`:
- Around line 102-130: The 5-minute hardcoded timeout in
provisionComponentSource (ctx, cancel :=
context.WithTimeout(context.Background(), 5*time.Minute)) should be extracted to
a named constant or configurable variable so it can be tuned (e.g.,
defaultProvisionTimeout or provisionTimeout) instead of being magic in-place;
change the ctx creation to use that constant (and update any related
docs/comments) and consider exposing it as a package-level var or config knob
used by provSource.AutoProvisionSource calls for easier CI tuning.
- Around line 79-97: The function resolveWorkdirComponentPath currently treats a
non-directory file at the candidate path as "doesn't exist"; change it to
surface an error instead: after os.Stat succeeds in resolveWorkdirComponentPath,
check fi.IsDir() and if false return the candidate path along with an explicit
error (wrap with errUtils.ErrWorkdirProvision and a message like "workdir
component path is not a directory") so callers like terraform_plan_diff.go and
terraform_verify_plan.go can fail fast and report misconfiguration rather than
silently falling back to componentPath.
- Around line 57-68: In applyWorkdirSubpathToSection, add a single-line
clarifying comment explaining that info.BaseComponentPath contains the
metadata.component value from the YAML config (set by ProcessComponentMetadata)
so readers understand why applyMetadataComponentSubpath(info.BaseComponentPath,
workdirPath) is correct; place the comment immediately above the call to
applyMetadataComponentSubpath (or above the function body near the use of
info.BaseComponentPath) and reference the symbols applyWorkdirSubpathToSection,
info.BaseComponentPath, applyMetadataComponentSubpath, and
provWorkdir.WorkdirPathKey in the note.
In `@tests/cli_source_provisioner_workdir_test.go`:
- Around line 114-154: The test currently discards the error returned by
cmd.Execute(), which can hide that provisioning never ran; change the code
around the cmd.Execute() call to capture its error (e.g., execErr :=
cmd.Execute()) and then, when asserting the workdir root exists (the
require.NoError checking statErr for workdirRoot), include execErr in the
failure message so the test output shows the Execute error if provisioning
didn't run; reference cmd.Execute() and the workdirRoot/statErr(require.NoError)
checks to locate where to capture and propagate the execution error into the
require.NoError failure message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a303f16-3334-4013-ae15-ed3a132de6cf
📒 Files selected for processing (3)
internal/exec/terraform_execute_helpers_test.gointernal/exec/terraform_provision_helpers.gotests/cli_source_provisioner_workdir_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/terraform_execute_helpers_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_execute_helpers_test.go`:
- Around line 712-756: The test uses assert.NotEmpty for candidate which is
weak; compute the expected candidate path the same way
resolveWorkdirComponentPath should (join atmosConfig.BasePath,
provWorkdir.WorkdirPath, cfg.TerraformComponentType, stack+"-"+componentName,
BaseComponentPath) using the values in
TestResolveWorkdirComponentPath_NonExistentDir (stack "dev", FinalComponent
"missing-component", BaseComponentPath "exports") and replace assert.NotEmpty(t,
candidate) with assert.Equal(t, expectedCandidate, candidate) (while keeping
require.NoError and assert.False for exists) so the test verifies the exact path
construction even when the directory does not exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59681a16-187e-45cf-886d-b129b53a44e1
📒 Files selected for processing (1)
internal/exec/terraform_execute_helpers_test.go
Replace assert.NotEmpty with assert.Equal against an explicitly constructed expected path so a regression in BuildPath or applyMetadataComponentSubpath wiring fails this test.
Address review feedback that surfaced AI-slop, missing test coverage,
and a sentinel-collision risk in the metadata.component subpath join.
Code:
- Trim 21-line docstring on applyMetadataComponentSubpath to 5 lines
and 8-line docstring on WorkdirSubpathAppliedKey to 3.
- Replace anonymous struct{}{} sentinel with a private
workdirSubpathAppliedMarker type. YAML deserialization cannot
produce this type, so a stack manifest containing
`_workdir_subpath_applied: true` can no longer bypass the join.
- Drop the always-discarded bool return from
applyWorkdirSubpathToSection.
- resolveWorkdirComponentPath now wraps ErrWorkdirProvision when the
candidate path exists but is not a directory, instead of silently
reporting exists=false.
Tests:
- Move helper tests into a co-located terraform_provision_helpers_test.go.
- Drop redundant _SingleSegment; rename _DotDotEscapeHatch to
_AllowsParentEscape so it frames the design contract instead of
filepath.Clean.
- Add _NoWorkdirPathKey, _EmptyWorkdirPath, _SentinelGatesDoubleJoin
(negative-path counterpart to _DoubleCallAppliesOnce),
_UserYAMLCannotForgeSentinel, and _RegularFileAtCandidate.
- Strengthen the integration test to glob for the generated varfile
and assert it lands at <workdir>/exports/, not the workdir root.
Reverting the fix moves the varfile and fails this test.
…erate varfile Two sibling code paths called AutoProvisionSource directly instead of going through provisionComponentSource, so they read WorkdirPathKey unmutated and dropped the metadata.component subpath join — same root cause as cloudposse#2364, just in different commands: - ExecuteTerraformShell (internal/exec/terraform_shell.go) — `atmos terraform shell` was dropping the user into the workdir root and writing the varfile there. Working directory, component path, and varfile path all now resolve to <workdir>/<metadata.component>/. - tryJITProvision (internal/exec/terraform_generate_varfile.go) — `atmos terraform generate varfile` was writing the varfile to the workdir root. Both fixes are a single applyWorkdirSubpathToSection call after the provisioner runs. The sentinel makes it idempotent if the component later goes through provisionComponentSource in the same invocation. Tests: - New TestJITSource_MetadataComponentSubpath_TerraformShell captures the dry-run banner from `atmos terraform shell --dry-run` and asserts Working directory and Component path include the subpath. - TestJITSource_MetadataComponentSubpath now drives `terraform generate varfile` (which actually writes the varfile, unlike `plan --dry-run`) and globs for the .terraform.tfvars.json to assert it lands at <workdir>/exports/, not the workdir root. Repro fixture verified end-to-end: \`atmos terraform shell iam-policy-jit -s demo --dry-run\` reports Working directory and Component path as \`.workdir/terraform/demo-iam-policy-jit/modules/iam-policy\`. Helmfile, Packer, and Ansible JIT components remain out of scope per the original PR description; a follow-up issue tracks them.
…workdir
The previous commits joined metadata.component onto the workdir path
unconditionally, which broke configs that use metadata.component as an
inheritance/identity pointer rather than as a real subdirectory inside
the cloned source repo. Concrete failure: a component with
`metadata.component: demo-cluster-codepipeline` whose source repo has
.tf files at the root caused atmos to look for a non-existent
`<workdir>/demo-cluster-codepipeline/` directory and bail out with
ErrInvalidTerraformComponent.
The two valid uses of metadata.component for JIT components are:
1. A real subdirectory inside the cloned repo where the Terraform
module lives (e.g. `modules/iam-policy` for terraform-aws-iam) —
issue cloudposse#2364, the original case this PR fixes.
2. An inheritance/identity pointer naming an abstract base component
when the cloned repo's .tf files live at its root.
String-shape disambiguation (e.g. presence of `/`) is unreliable; the
filesystem already encodes the answer. After the source provisioner has
cloned, either the joined directory exists (case 1) or it doesn't
(case 2). The new resolveWorkdirSubpath helper does that check and is
shared between applyWorkdirSubpathToSection (live execution) and
resolveWorkdirComponentPath (plan-diff/verify-plan). When the joined
path doesn't exist, both paths fall back to the workdir root and log a
debug message; non-ENOENT stat failures still surface as wrapped
ErrWorkdirProvision so corrupt state isn't masked.
Tests:
- New TestResolveWorkdirSubpath_* unit tests cover the pure existence-
check helper (joined exists, joined missing, empty subpath, non-dir
at candidate).
- New TestApplyWorkdirSubpathToSection_InheritancePointerPreservesRoot
is the regression guard for the inheritance-pointer case the previous
commits broke.
- New TestResolveWorkdirComponentPath_InheritancePointerFallsBack
covers the same case for plan-diff/verify-plan.
- Existing tests updated to mkdir the subpath where they expect a join
(the helper now reads the filesystem; t.TempDir() alone no longer
satisfies the precondition).
Contributor
Author
|
CodeRabbit (@coderabbitai) scope has grown a tad to deal with other JIT/workdir terraform wonkiness, can you give another review |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/exec/terraform_provision_helpers_test.go (2)
90-95: Add a compile-time sentinel forschema.ConfigAndStacksInfofield usage.These tests depend on specific struct fields; add one compile guard near the top of the file so field renames fail fast at compile time.
🛡️ Suggested compile-time guard.
package exec import ( @@ "github.com/cloudposse/atmos/pkg/schema" ) +var _ = schema.ConfigAndStacksInfo{ + BaseComponentPath: "", + FinalComponent: "", + Stack: "", + ComponentSection: map[string]any{}, +} + // applyMetadataComponentSubpath ─────────────────────────────────────────────.As per coding guidelines "Add compile-time sentinels for schema field references in tests: when a test uses a specific struct field (e.g.,
schema.Provider{Kind: "azure"}), addvar _ = schema.Provider{Kind: "azure"}as a compile guard so a field rename immediately fails the build."Also applies to: 251-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_provision_helpers_test.go` around lines 90 - 95, Add a compile-time sentinel for the struct fields used in these tests so renames break the build: near the top of the test file declare a blank-variable initializer using schema.ConfigAndStacksInfo that sets the exact fields referenced in the tests (e.g., BaseComponentPath and ComponentSection with the provWorkdir.WorkdirPathKey key) — this creates a compile-time guard that will fail if those field names are renamed; place a similar sentinel for the other occurrence around lines 251-257 to cover both usages.
21-376: Consider consolidating repeated scenarios into table-driven subtests.The suite is solid, but a table-driven structure would reduce repetition and make future scenario additions cheaper.
As per coding guidelines "Use table-driven tests for comprehensive coverage; prefer unit tests with mocks over integration tests; target >80% coverage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_provision_helpers_test.go` around lines 21 - 376, Several tests repeat the same setup/assert patterns; convert them into table-driven subtests to reduce duplication: group similar cases for applyMetadataComponentSubpath (tests referencing applyMetadataComponentSubpath_*), resolveWorkdirSubpath (resolveWorkdirSubpath_*), applyWorkdirSubpathToSection (applyWorkdirSubpathToSection_*), and resolveWorkdirComponentPath (ResolveWorkdirComponentPath_*). For each group, create a slice of test cases with name, inputs (e.g., BaseComponentPath, workdir, ComponentSection, atmosConfig, etc.), expected outputs and error flags, then iterate with t.Run to perform the shared setup, call the target function (applyMetadataComponentSubpath, resolveWorkdirSubpath, applyWorkdirSubpathToSection, resolveWorkdirComponentPath) and assert expectations; preserve special negative-path checks (sentinel forging, double-call idempotency, permission/ENONET vs EACCES behavior) as individual cases in the table or separate subtests where behavior cannot be expressed in a simple case row. Ensure unique sentinel assertions (workdirSubpathAppliedMarker) remain explicit in the corresponding table entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_provision_helpers.go`:
- Around line 139-143: The raw errors returned from u.IsDirectory should be
wrapped with the package's static sentinel error (from errors/errors.go) before
returning so callers can reliably classify them; replace direct returns of err
after calling u.IsDirectory with a wrapped error using the sentinel (e.g. wrap
via fmt.Errorf("%w: %v", errors.Err<appropriateName>, err) or
errors.Join(errors.Err<appropriateName>, err)) in the branch that checks
provSource.HasSource(info.ComponentSection) (the block using u.IsDirectory and
returning componentPath, exists, err) and do the same for the other
u.IsDirectory call later in this file that returns early, ensuring you reference
the sentinel error symbol from errors/errors.go (and use errors.Join if
combining multiple errors).
- Around line 145-149: The current use of context.Background() in
provisionComponentSource loses upstream cancellation; update the function
signature of provisionComponentSource to accept a context.Context and replace
context.Background() with the passed ctx (and keep the local timeout via
context.WithTimeout(ctx,...)). Then thread that ctx through the call chain by
updating resolveAndProvisionComponentPath, prepareComponentExecution, and
ExecuteTerraform to accept a context.Context parameter and pass the received ctx
down to their callers (preserving existing timeout/cancel where needed and
retaining error handling). Ensure all calls to provSource.AutoProvisionSource
use the propagated ctx so cancellations/deadlines from ExecuteTerraform
propagate end-to-end.
---
Nitpick comments:
In `@internal/exec/terraform_provision_helpers_test.go`:
- Around line 90-95: Add a compile-time sentinel for the struct fields used in
these tests so renames break the build: near the top of the test file declare a
blank-variable initializer using schema.ConfigAndStacksInfo that sets the exact
fields referenced in the tests (e.g., BaseComponentPath and ComponentSection
with the provWorkdir.WorkdirPathKey key) — this creates a compile-time guard
that will fail if those field names are renamed; place a similar sentinel for
the other occurrence around lines 251-257 to cover both usages.
- Around line 21-376: Several tests repeat the same setup/assert patterns;
convert them into table-driven subtests to reduce duplication: group similar
cases for applyMetadataComponentSubpath (tests referencing
applyMetadataComponentSubpath_*), resolveWorkdirSubpath
(resolveWorkdirSubpath_*), applyWorkdirSubpathToSection
(applyWorkdirSubpathToSection_*), and resolveWorkdirComponentPath
(ResolveWorkdirComponentPath_*). For each group, create a slice of test cases
with name, inputs (e.g., BaseComponentPath, workdir, ComponentSection,
atmosConfig, etc.), expected outputs and error flags, then iterate with t.Run to
perform the shared setup, call the target function
(applyMetadataComponentSubpath, resolveWorkdirSubpath,
applyWorkdirSubpathToSection, resolveWorkdirComponentPath) and assert
expectations; preserve special negative-path checks (sentinel forging,
double-call idempotency, permission/ENONET vs EACCES behavior) as individual
cases in the table or separate subtests where behavior cannot be expressed in a
simple case row. Ensure unique sentinel assertions (workdirSubpathAppliedMarker)
remain explicit in the corresponding table entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8006416a-5f54-49ab-80cc-70cf82861701
📒 Files selected for processing (11)
internal/exec/terraform_execute_helpers.gointernal/exec/terraform_generate_varfile.gointernal/exec/terraform_plan_diff.gointernal/exec/terraform_provision_helpers.gointernal/exec/terraform_provision_helpers_test.gointernal/exec/terraform_shell.gointernal/exec/terraform_verify_plan.gopkg/provisioner/workdir/types.gotests/cli_source_provisioner_workdir_test.gotests/fixtures/scenarios/source-provisioner-workdir/stacks/catalog/source-with-workdir-metadata-component.yamltests/fixtures/scenarios/source-provisioner-workdir/stacks/deploy/dev.yaml
💤 Files with no reviewable changes (1)
- internal/exec/terraform_execute_helpers.go
Address CodeRabbit feedback: u.IsDirectory's raw os.PathError leaked through the !HasSource and post-provision return paths in provisionComponentSource, inconsistent with the workdir branch which already wrapped with the errUtils.ErrWorkdirProvision sentinel. Extract componentDirExists helper so all three IsDirectory call sites share the same wrap-on-non-ENOENT semantics. ENOENT is communicated through the exists boolean (and err=nil); other stat failures are wrapped for upstream classification. Refactor also keeps provisionComponentSource under the revive cyclomatic-complexity ceiling.
Contributor
Author
|
Addressed CodeRabbit nitpick from review 4190754834 in commit a9ca466d3: Compile-time sentinels for Table-driven refactor — deferring this as out of scope for this bug-fix PR. The tests are straightforward flat tests and converting them to table-driven would create a large, hard-to-review diff that obscures the actual bug fix. The special-case tests (sentinel forging, double-call idempotency, permission/ENOENT vs EACCES) don't fit cleanly into a shared row schema without adding nearly as much boilerplate as the current tests. Will track as a follow-up cleanup. Context threading — already acknowledged and declined in the prior thread (9 existing sites follow the local-timeout pattern; threading through only |
Adds blank-variable initialisers for schema.ConfigAndStacksInfo and schema.AtmosConfiguration so any rename of BaseComponentPath, FinalComponent, Stack, ComponentSection, or BasePath immediately fails the build and flags the affected assertions. Addresses CodeRabbit nitpick on PR cloudposse#2371.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_provision_helpers.go`:
- Around line 51-56: In resolveWorkdirSubpath, reject absolute
metadataComponentSubpath values before calling filepath.Join to prevent an
absolute path from bypassing workdirRoot; add a check using
filepath.IsAbs(metadataComponentSubpath) and return a clear error (including the
offending path) if true, while preserving support for relative paths and parent
segments like "..", then proceed with filepath.Join(workdirRoot,
metadataComponentSubpath) and os.Stat as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac6bf1e7-fbb0-46db-bc3f-25add26e5c7f
📒 Files selected for processing (2)
internal/exec/terraform_provision_helpers.gointernal/exec/terraform_provision_helpers_test.go
Per CodeRabbit review on PR cloudposse#2371: metadata.component is contractually a relative subpath inside the provisioned workdir. filepath.Join cleans absolute paths into a child of workdirRoot on Unix (so the join is non-escaping) and handles drive letters specially on Windows, but in either case an absolute value is nonsensical input. Failing fast with ErrWorkdirProvision surfaces the author error instead of silently coercing it.
…5 entry points (cloudposse#2364) Address maintainer feedback on PR cloudposse#2371: helpers belong in pkg/, and the metadata.component subpath fix should not be curve-fitted to terraform. New package pkg/component/ owns the path-resolution orchestrator: - ResolveWorkdirSubpath / ApplyWorkdirSubpathToSection: pure path logic + in-place ComponentSection mutation (idempotent via private-typed sentinel). - BuildAndResolveWorkdirPath: post-ProcessStacks resolver used by plan-diff and verify-plan when ComponentSection has been rebuilt without WorkdirPathKey. - ProvisionAndResolveComponentPath: full orchestrator (existence check -> AutoProvisionSource -> Apply -> re-check), parameterized by componentType. Five executor entry points collapse to one orchestrator call each: - internal/exec/terraform_execute_helpers.go (plan/apply/etc.) - internal/exec/terraform_generate_varfile.go (replaces tryJITProvision + checkDirectoryExists, ~130 lines deleted plus 5 redundant unit tests). - internal/exec/helmfile.go - internal/exec/packer.go - pkg/component/ansible/executor.go terraform_plan_diff.go and terraform_verify_plan.go now call component.BuildAndResolveWorkdirPath; terraform_shell.go and terraform_generate_varfile.go also call ApplyWorkdirSubpathToSection at the post-ProcessStacks boundary. Adjacent behavior change: helmfile/packer/ansible/terraform-generate-varfile used to short-circuit JIT when the local fallback dir existed. The orchestrator now runs AutoProvisionSource unconditionally when source.uri is declared, matching terraform plan/apply's pre-existing behavior. AutoProvisionSource self-debounces via invocationDoneKey + needsProvisioning so steady-state runs are still cheap. Error sentinel precision (three sentinels for three classes): - ErrProvisionerFailed: AutoProvisionSource hook failed. - ErrWorkdirProvision: workdir path / stat / abs-subpath rejection failed. - ErrInvalidComponent: stat failed on the local component dir (no-source fallback). componentDirExists takes the sentinel as a parameter so the classification matches the actual path. terraform_shell.go no longer re-wraps an already-classified ErrWorkdirProvision with ErrProvisionerFailed. Test polish: - Pipe drainer in TestJITSource_MetadataComponentSubpath_TerraformShell now starts BEFORE cmd.Execute to avoid a deadlock window when the OS pipe buffer fills. Order: w.Close() -> <-drained -> r.Close(). - Tightened the suffix assertion to anchor on the "Working directory:" line instead of any line in the output. - Replaced slash-string filepath.Join violation in TestBuildAndResolveWorkdirPath_AllComponentTypesWithSubpath per CLAUDE.md. Constant relocation: - Renamed and unexported WorkdirSubpathAppliedKey -> workdirSubpathAppliedKey; moved from pkg/provisioner/workdir/types.go to pkg/component/workdir_path.go. The key is read/written exclusively by pkg/component, so the protocol now lives next to the only code that uses it. Removed redundant blank imports of pkg/provisioner/source from helmfile.go / packer.go / ansible/executor.go (the init runs transitively via pkg/component). 24 unit tests in pkg/component/workdir_path_test.go (parametric over all four componentTypes, negative-path sentinel checks, YAML-author forge resistance, EACCES propagation). Two integration tests in tests/cli_source_provisioner_workdir_test.go (load-bearing assertions; both fail when the fix is reverted). Fixes cloudposse#2364.
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
…call sites Per CLAUDE.md "Don't reference the current task, fix, or callers": the introductory comment block in front of each component.ProvisionAndResolveComponentPath call site explained "this is the shared orchestrator so X honors metadata.component the same way terraform does (issue cloudposse#2364)". That framing rots the moment any of the executors diverges, and it duplicates what the function name already says. - helmfile.go / packer.go / ansible/executor.go: drop the block; the call is self-documenting. - terraform_execute_helpers.go: tighten to the actual ordering invariant ("provision before generate so files land in the workdir"). - terraform_shell.go: tighten to the ordering invariant ("ExecuteProvisioners sets the key, this consumes it"). - terraform_generate_varfile.go: ensureTerraformComponentExists godoc trimmed to the API contract. - pkg/component/workdir_path.go: ResolveWorkdirSubpath godoc keeps the inheritance-pointer disambiguation (it documents the API contract) but drops the issue reference. Test-side comments still reference cloudposse#2364 because that's what the tests are regression-guarding; that is the legitimate use of an issue number in code.
zack-is-cool
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 4, 2026 20:28
03fef59
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/component/workdir_path.go`:
- Around line 166-171: componentDirExists currently treats existing regular
files the same as missing paths because u.IsDirectory returns (false, nil) for
files; add an explicit check after the u.IsDirectory call to return an error
when exists is false but no ErrNotExist was returned. Update componentDirExists
to, when err == nil and exists == false, return false with an error wrapping the
sentinel and a message like "<contextLabel> <componentPath>: not a directory"
(mirroring the fi.IsDir() check pattern used in BuildAndResolveWorkdirPath) so
callers can distinguish "path exists but is not a directory" from "path
missing".
- Around line 64-66: The check that rejects absolute subpaths currently uses
filepath.IsAbs(metadataSubpath) but misses Windows volume-qualified relative
paths like "C:modules\..."—update the validation in the function handling
metadataSubpath to also detect and reject volume-qualified paths by calling
filepath.VolumeName(metadataSubpath) and treating a non-empty volume name (e.g.,
"C:") as invalid; when detected, return the same combined error
(errors.Join(errUtils.ErrWorkdirProvision, fmt.Errorf(...))) referencing
metadataSubpath so the message remains consistent with the existing branch that
handles absolute paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdf6f2d8-d663-4549-8b68-7507caef67b8
📒 Files selected for processing (7)
internal/exec/helmfile.gointernal/exec/packer.gointernal/exec/terraform_execute_helpers.gointernal/exec/terraform_generate_varfile.gointernal/exec/terraform_shell.gopkg/component/ansible/executor.gopkg/component/workdir_path.go
✅ Files skipped from review due to trivial changes (2)
- internal/exec/terraform_shell.go
- internal/exec/helmfile.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/exec/packer.go
- internal/exec/terraform_execute_helpers.go
- pkg/component/ansible/executor.go
- internal/exec/terraform_generate_varfile.go
Contributor
Author
|
CodeRabbit (@coderabbitai) scope grew significantly since your last pass:
Please give it another full review. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainOn it — pulling all the key files now for a full pass. 🏁 Script executed: Length of output: 78536
pkg/component/workdir_path.go All five call sites — confirmed wired correctly:
(That's actually eight — all five JIT-capable executor families covered.) Dead code — confirmed absent: Tests:
One stale comment (minor, not a blocker): The godoc on Nothing else to flag. This is ready. [approve] |
…rectory fallbacks Two CodeRabbit findings on the orchestrator helpers: 1. ResolveWorkdirSubpath now rejects volume-qualified relative paths like `C:modules\iam-policy` on Windows. filepath.IsAbs returns false for such inputs (they are drive-relative, interpreted against the drive's current directory), so filepath.VolumeName is checked alongside IsAbs. 2. componentDirExists now stats directly instead of delegating to u.IsDirectory, which collapsed an existing regular file into the same (false, nil) signal as ENOENT. A regular file at the workdir or fallback path is now surfaced as a wrapped error matching the !fi.IsDir() guards already used in ResolveWorkdirSubpath and BuildAndResolveWorkdirPath, so callers see "exists but is not a directory" instead of the misleading "does not exist". Adds unit tests for both: the Windows test runs only on GOOS=windows (filepath.VolumeName is OS-specific), and a no-source regular-file test confirms the ErrInvalidComponent classification path.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (56.54%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2371 +/- ##
==========================================
+ Coverage 78.04% 78.13% +0.08%
==========================================
Files 1091 1092 +1
Lines 103248 103308 +60
==========================================
+ Hits 80585 80723 +138
+ Misses 18244 18156 -88
- Partials 4419 4429 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Andriy Knysh (aknysh)
approved these changes
May 4, 2026
Andriy Knysh (aknysh)
left a comment
Andriy Knysh (aknysh)
left a comment
Member
There was a problem hiding this comment.
thanks zack-is-cool
Contributor
|
These changes were released in v1.217.0. |
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
3 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
JIT-provisioned components can now point at a submodule inside a cloned upstream repo via
metadata.component, the same way non-JIT components already can. Every JIT-capable code path is covered —terraform plan/apply,terraform generate varfile,terraform shell,helmfile,packer, andansible— and the resolver now lives inpkg/component/rather than being curve-fitted to Terraform insideinternal/exec/.Before this PR,
metadata.component: modules/iam-policywas silently ignored on the JIT/workdir code path — atmos cloned the repo to.workdir/<type>/<stack>-<component>/and ran the underlying tool against that root, so generated files (backend.tf.json, varfile,.terraform/, helmfile state, packer cache, ansible inventory) all landed at the repo root instead of at.workdir/<type>/<stack>-<component>/modules/iam-policy/. The tools then either failed with confusing errors or silently ran against the wrong directory (e.g. a repo root with no.tffiles).Fixes #2364.
Why this matters
Some upstream repos organize modules under
modules/<name>/rather than at the repo root (e.g.terraform-aws-modules/terraform-aws-iam). For JIT to be useful against those repos, atmos needs to clone the whole repo into a workdir (so relative parent-path references like../../shared-vars.tfresolve) and then run the tool against a specific submodule inside it. Non-JIT components already support this viametadata.component; this PR brings JIT/non-JIT parity for that capability and applies it uniformly across every executor.What changed (revised after maintainer review)
This PR was originally scoped to Terraform with helpers in
internal/exec/. Maintainer feedback on the first review was clear: the helpers belong inpkg/, and the same fix should apply to Helmfile/Packer/Ansible — not be curve-fitted to Terraform.Both items are addressed in this revision.
New package:
pkg/component/Helpers extracted into a single component-type-parameterized package, used by all four executors:
All five executor entry points collapse to one call
internal/exec/terraform_execute_helpers.go(plan/apply/etc.)provisionComponentSource(...)(terraform-only helper)component.ProvisionAndResolveComponentPath(ctx, ..., cfg.TerraformComponentType, ...)internal/exec/terraform_generate_varfile.gotryJITProvision(...)+ privatecheckDirectoryExists(terraform-only reimpl, subpath bolted on)component.ProvisionAndResolveComponentPath(ctx, ..., cfg.TerraformComponentType, ...)internal/exec/helmfile.go(prev. lines 121-155)provSource.AutoProvisionSource+ rawWorkdirPathKeylookup (subpath ignored)component.ProvisionAndResolveComponentPath(ctx, ..., cfg.HelmfileComponentType, ...)internal/exec/packer.go(prev. lines 121-155)component.ProvisionAndResolveComponentPath(ctx, ..., cfg.PackerComponentType, ...)pkg/component/ansible/executor.go(prev. lines 380-429)component.ProvisionAndResolveComponentPath(ctx, ..., cfg.AnsibleComponentType, ...)Helmfile, Packer, Ansible, and
terraform generate varfilesilently inherited the same #2364 bug; this PR fixes them at the same time as Terraform plan/apply, with zero curve-fitting.terraform shelland the post-ProcessStacksresolvers (terraform plan-diff,terraform verify-plan) callcomponent.ApplyWorkdirSubpathToSectionandcomponent.BuildAndResolveWorkdirPathrespectively for the same reason.Adjacent behavior change: JIT runs whenever
source.uriis setBefore this PR, helmfile/packer/ansible/
terraform generate varfileonly invokedAutoProvisionSourcewhen the local fallback component dir was missing. Ansible andterraform generate varfileadditionally short-circuited the moment that dir existed, never running JIT. After the refactor, all five entry points (terraform plan/apply, terraform generate varfile, helmfile, packer, ansible) take the same path: whensource.uriis declared, the source provisioner runs unconditionally, and only the YAML'ssource.uridecides whether JIT is in play.This is safe under steady-state operation because
AutoProvisionSourcealready self-debounces via two cache layers —invocationDoneKey(no-ops a second call within the same command lifecycle) andneedsProvisioning(skips re-provisioning when the version, URI, and freshness pin all match) — both inpkg/provisioner/source/provision_hook.go. Net effect for users: every JIT-capable entry point now honorssource.urithe same way terraform plan/apply always has, and the previously preferred lazy-skip-on-stale-local-dir path is gone.Post-ProcessStacks resolvers also use the shared helper
internal/exec/terraform_plan_diff.goandinternal/exec/terraform_verify_plan.gopreviously called the terraform-privateresolveWorkdirComponentPath; both now callcomponent.BuildAndResolveWorkdirPath(atmosConfig, info, cfg.TerraformComponentType).Existence-gated subpath join (the disambiguation)
metadata.componenthas two valid uses for JIT components — a real subdirectory inside the cloned repo, or an inheritance/identity pointer to an abstract base. The fix consults the filesystem rather than guessing from the string. After clone, either the joined subdirectory exists (case 1, apply the join) or it doesn't (case 2, leave the workdir root alone). String-shape heuristics (e.g. checking for/) are unreliable; the filesystem already encodes the right answer aftergit clone.An unexported
workdirSubpathAppliedKeyconstant + privatesubpathAppliedMarkerstruct type (both inpkg/component/) guard against double-joining if the orchestrator is invoked twice against the sameinfo.ComponentSectionmap. YAML deserialization can't produce this type, so a stack manifest containing_workdir_subpath_applied: <anything>cannot bypass the join. The constant lives inpkg/component/rather thanpkg/provisioner/workdir/because read/write access is confined to this package — keeping the protocol single-sourced next to the only code that uses it.Error sentinel precision
Three sentinels carry distinct meaning across the orchestrator and its callers:
errUtils.ErrProvisionerFailed—AutoProvisionSource(the JIT hook) failed.errUtils.ErrWorkdirProvision— path resolution / stat / abs-subpath rejection failure on the workdir path.errUtils.ErrInvalidComponent— stat failure on the local component directory (the no-source fallback path).Two related fixes during review:
AutoProvisionSourcefailures withErrWorkdirProvision, which conflicted with the established pattern inpkg/provisioner/registry.go,internal/exec/terraform_shell.go, and the (now-removed) ansible executor — all of which usedErrProvisionerFailed. Bringing ansible's existing semantics back.componentDirExistshelper used to wrap every stat failure withErrWorkdirProvision, including the!HasSourcebranch where the path is a local component directory, not a workdir. It now takes a sentinel parameter so the wrap matches the actual classification.terraform_shell.gono longer re-wraps an already-wrappedErrWorkdirProvisionwithErrProvisionerFailed; the original sentinel survives in the chain soerrors.Istriage works correctly.Design notes
..inmetadata.componentis allowed. Many upstream Terraform modules reference shared files via relative parent paths (../../shared-vars.tf) and need the full repo on disk with the working directory at a subdirectory. Restricting to strict subpaths would break those layouts. Atmos's threat model assumes a trusted operator running atmos against their own stack configs —metadata.componentis YAML-author-controlled, on par with!exec,!template, and!terraform.state, all of which can read or invoke arbitrary host resources. The godoc onResolveWorkdirSubpathspells this out for future readers.Absolute
metadata.componentis rejected. An absolute value violates the documented contract (metadata.componentis a relative subpath inside the workdir).filepath.Joinwould silently coerce it into a child of the workdir root on Unix and apply drive-letter semantics on Windows — coercing it would mask author error. Rejected up-front with a wrappedErrWorkdirProvision.Same-name inheritance is a no-op. When
metadata.componentequals the component instance name (e.g. a component namedvpcwithmetadata.component: vpc), atmos already clears the field during stack processing, soinfo.BaseComponentPathis empty andfilepath.Joinis never called.Trade-off: typos fall through. A typo in
metadata.component(e.g.modules/iam-policwhen the user meantmodules/iam-policy) silently falls back to the workdir root rather than failing fast. This matches pre-PR behavior for invalid subpaths and is logged at debug level for traceability. A future enhancement could surface a warning when the join falls back, distinguishing typos from intentional inheritance-pointer use.What is not changed
The orchestrator short-circuits at
!provSource.HasSource(...), so non-source components never reach the new code. Behavior of each non-JIT shape:metadata.component<workdirRoot>/<subpath>doesn't exist on disk; the resolver returnsexists=falseand the originalcomponentPathis preserved.metadata.componentplan-diffandverify-planonly: these two commands now resolve to the provisioned workdir root (where state actually lives) instead of falling back to the local component path. Live execution paths were already correct.Other deferred scope:
atmos terraform generate planfile— this command buildscomponentPathdirectly fromatmosConfig.TerraformDirAbsolutePath + info.FinalComponentand never invokes JIT provisioning. As a result,generate planfiledoes not support JIT components today (with or without ametadata.componentsubpath) — a pre-existing limitation, not a regression introduced by this PR. Wiring up JIT provisioning there is out of scope.Tests
Unit (
pkg/component/workdir_path_test.go):TestResolveWorkdirSubpath_JoinedPathExistsTestResolveWorkdirSubpath_JoinedPathMissingFallsBackTestResolveWorkdirSubpath_EmptySubpathReturnsRootmetadata.componentshort-circuits to the rootTestResolveWorkdirSubpath_AllowsParentSegment..resolves naturally — codifies the design decisionTestResolveWorkdirSubpath_RejectsAbsolutePathErrWorkdirProvisionTestResolveWorkdirSubpath_RejectsAbsolutePathOutsideWorkdirTestResolveWorkdirSubpath_RegularFileAtCandidateErrWorkdirProvisionwhen the candidate exists but is not a directoryTestApplyWorkdirSubpathToSection_JoinsSubpathWorkdirPathKeyto the joined subpath and sets the typed sentinelTestApplyWorkdirSubpathToSection_InheritancePointerPreservesRootWorkdirPathKeystays at the workdir rootTestApplyWorkdirSubpathToSection_NoWorkdirPathKeyWorkdirPathKeyis absentTestApplyWorkdirSubpathToSection_EmptyWorkdirPathWorkdirPathKeyis the empty stringTestApplyWorkdirSubpathToSection_DoubleCallAppliesOnceTestApplyWorkdirSubpathToSection_SentinelGatesDoubleJoinTestApplyWorkdirSubpathToSection_UserYAMLCannotForgeSentinelbool,string,int,map) cannot impersonate the typed sentinelTestBuildAndResolveWorkdirPath_ExistingDir(joined-path, true, nil)when the workdir subpath existsTestBuildAndResolveWorkdirPath_AllComponentTypes.workdir/<componentType>/TestBuildAndResolveWorkdirPath_AllComponentTypesWithSubpathmetadata.componentsubpath join (issue #2364 across executors)TestBuildAndResolveWorkdirPath_InheritancePointerFallsBack(workdir-root, true, nil)when the workdir root exists but the subpath doesn'tTestBuildAndResolveWorkdirPath_NonExistentDir(candidate, false, nil)when the workdir is not provisioned yetTestBuildAndResolveWorkdirPath_RegularFileAtCandidateErrWorkdirProvisionwhen the candidate exists but is not a directoryTestBuildAndResolveWorkdirPath_StatErrorPropagatesErrWorkdirProvisionfor non-ENOENTstat failures (EACCES)TestProvisionAndResolveComponentPath_NoSourceReturnsFallbackTestProvisionAndResolveComponentPath_NoSourceMissingDirexists=falsecorrectly when fallback dir is absentIntegration (
tests/cli_source_provisioner_workdir_test.go):TestJITSource_MetadataComponentSubpathatmos terraform generate varfile null-label-exports -s dev*.terraform.tfvars.jsonlands at<workdir>/exports/, not the workdir root. Reverting the fix moves the varfile and fails this assertion.TestJITSource_MetadataComponentSubpath_TerraformShellatmos terraform shell null-label-exports -s dev --dry-runWorking directoryandComponent pathinclude themetadata.componentsubpath. Reverting the fix prints the bare workdir root and fails this assertion.Both use
github.com/cloudposse/terraform-null-label@0.25.0withmetadata.component: exportsand skip gracefully on offline runners (no GitHub access or nogitbinary) via the existing precondition helpers.References
Summary by CodeRabbit
New Features
Bug Fixes
Tests