fix(vendor): resolve relative vendor targets against the Atmos base_path - #2833
fix(vendor): resolve relative vendor targets against the Atmos base_path#2833thejrose1984 wants to merge 17 commits into
Conversation
When `atmos.yaml` declares the vendor manifest location via `vendor.base_path`, relative `targets` were joined to the directory containing `vendor.yaml`. Keeping `atmos.yaml` and `vendor.yaml` in a config directory therefore dropped the vendored `components/terraform` tree next to the manifest instead of under the Atmos `base_path`, where `components.terraform.base_path` and `stacks.base_path` point. This is the behavior the docs already described. Targets now resolve against the Atmos `base_path` whenever the manifest location comes from `vendor.base_path`. A manifest discovered in the working directory (no `vendor.base_path` configured) keeps resolving targets relative to itself, so the common single-directory layout is unchanged. Local `source` paths still resolve relative to the manifest that declares them. Also honor absolute `targets` as-is; `filepath.Join` was nesting them under the base path even though absolute targets are documented as supported. Fixes cloudposse#2409
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
|
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:
📝 WalkthroughWalkthroughVendor pull now resolves relative targets against Atmos ChangesVendor path resolution
Test environment portability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VendorPull
participant VendorUtils
participant VendorManifest
participant Filesystem
VendorPull->>VendorUtils: Merge vendor sources
VendorUtils->>VendorManifest: Read declaring manifest directory
VendorUtils->>VendorUtils: Resolve source and target paths
VendorUtils->>Filesystem: Write vendored artifacts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/exec/vendor_utils_test.go`:
- Around line 1002-1038: Update both test sites in
internal/exec/vendor_utils_test.go (lines 1002-1038 and 1080-1111) to derive
absTarget from t.TempDir() instead of joining filepath.Separator with fixed path
components. Use that real platform-specific absolute path in each target-path
test while preserving the existing expected behavior and assertions.
🪄 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 Plus
Run ID: f669d52f-2541-45f2-bce7-cc5d36169f0d
📒 Files selected for processing (6)
internal/exec/vendor_pull_integration_test.gointernal/exec/vendor_utils.gointernal/exec/vendor_utils_test.gowebsite/docs/cli/commands/vendor/vendor-pull.mdxwebsite/docs/cli/configuration/vendor.mdxwebsite/docs/vendor/config/sources.mdx
`filepath.Join(string(filepath.Separator), "opt", ...)` produces `\opt\...` on Windows, which `filepath.IsAbs` rejects because it carries no volume name. The absolute-target assertions would therefore have exercised the relative branch and failed on Windows CI. Derive the path from `t.TempDir()` instead, and guard the precondition with `require.True(filepath.IsAbs(...))` so a future regression fails loudly rather than silently testing the wrong branch. Also add doc comments to the new test functions.
|
CodeRabbit (@coderabbitai) please reconcile against our PRDs for relative path handling |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="cloudposse/atmos"
echo "== Issue `#2409` =="
gh issue view 2409 --repo "$repo" --comments
echo
echo "== PR `#2833` metadata and comments =="
gh pr view 2833 --repo "$repo" --comments
echo
echo "== Locate potential PRD/relative-path documentation =="
rg -n -i -C 2 'PRD|product requirements|relative path|base_path|vendor\.base_path|vendor\.yaml' \
README.md docs website internal 2>/dev/null || true
echo
echo "== Resolver outline =="
ast-grep outline internal/exec/vendor_utils.go --items all --view expanded
echo
echo "== Relevant resolver implementation =="
rg -n -C 12 'resolveVendorTargetBasePath|resolveVendorTargetPath|TargetBasePath|VendorConfigFilePath' \
internal/exec/vendor_utils.go
echo
echo "== Changed documentation =="
for f in \
website/docs/cli/commands/vendor/vendor-pull.mdx \
website/docs/cli/configuration/vendor.mdx \
website/docs/vendor/config/sources.mdx
do
echo "--- $f ---"
rg -n -C 8 'target|base_path|relative|absolute' "$f" || true
doneLength of output: 200937 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="cloudposse/atmos"
echo "== Issue `#2409`, bounded =="
gh issue view 2409 --repo "$repo" --json number,title,body,url,comments \
--jq '{number,title,url,body,comments:[.comments[]|{author:.author.login,body,url}]}'
echo
echo "== PR `#2833`, bounded =="
gh pr view 2833 --repo "$repo" --json number,title,body,url,comments,reviewThreads \
--jq '{number,title,url,body,comments:[.comments[]|{author:.author.login,body,url}],reviewThreads:[.reviewThreads[]|{isResolved,comments:[.comments[]|{author:.author.login,body,path,line,url}]}]}'
echo
echo "== Candidate PRDs =="
fd -t f . docs/prd 2>/dev/null | sort || true
rg -n -i -C 4 'vendor|relative path|base_path|target' docs/prd 2>/dev/null || true
echo
echo "== Import processing and local-source resolution =="
sed -n '500,640p' internal/exec/vendor_utils.go
echo "--- unit test outline ---"
ast-grep outline internal/exec/vendor_utils_test.go --items all --view expanded
echo "--- import/local-source tests and calls ---"
rg -n -C 8 'processVendorImports|determineSourceType|SourceIsLocalFile|local.*source|imports?' \
internal/exec/vendor_utils_test.go internal/exec/vendor_pull_integration_test.goLength of output: 6572 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="cloudposse/atmos"
echo "== PR `#2833` =="
gh pr view 2833 --repo "$repo" --json number,title,body,url,comments,reviews,files \
--jq '{number,title,url,body,comments:[.comments[]|{author:.author.login,body,url}],reviews:[.reviews[]|{author:.author.login,state,body,url}],files:[.files[].path]}'
echo
echo "== Inline review comments =="
gh api "repos/${repo}/pulls/2833/comments?per_page=100" \
--jq '.[] | {user:.user.login, path, line, body, html_url}' || true
echo
echo "== Import behavior: implementation =="
sed -n '245,395p' internal/exec/vendor_utils.go
sed -n '500,555p' internal/exec/vendor_utils.go
sed -n '584,625p' internal/exec/vendor_utils.go
echo
echo "== Import/local-source coverage =="
rg -n -C 10 'processVendorImports|imports:|SourceIsLocalFile|determineSourceType|source:.*(\./|\.\./)|local' \
internal/exec/vendor_utils_test.go internal/exec/vendor_pull_integration_test.go
echo
echo "== PRD candidate files =="
fd -t f . docs/prd 2>/dev/null | sort | head -n 200Length of output: 60742 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Base-path semantics PRD =="
rg -n -C 8 -i 'relative|absolute|base.path|manifest|vendor|source|target|import' \
docs/prd/base-path-resolution-semantics.md
echo
echo "== Import provenance PRDs =="
for f in docs/prd/import-provenance.md docs/prd/provenance-import-resolution.md; do
echo "--- $f ---"
rg -n -C 8 -i 'relative|absolute|path|manifest|declaring|provenance|source|target|import' "$f"
done
echo
echo "== Existing provenance model =="
rg -n -C 6 'AtmosVendorSource|\.File\b|SourceFile|Origin|Provenance|declaring manifest' \
internal pkg | head -n 500Length of output: 91884
Aligned
Gap — imported local sources
Thus, if This conflicts with the PRD’s core convention: a relative path defined in a configuration file is anchored to the directory of the file that declares it. The target behavior is correct; imported-source provenance needs to be carried into source resolution, with coverage for nested imports. You are interacting with an AI system. |
thejrose1984 please analyze this review and address the concerns |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2833 +/- ##
=======================================
Coverage 82.18% 82.19%
=======================================
Files 1802 1803 +1
Lines 175214 175239 +25
=======================================
+ Hits 144005 144040 +35
+ Misses 23496 23485 -11
- Partials 7713 7714 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…fest Sources merged in from `spec.imports` were all resolved against the root manifest's directory. A manifest at `vendor/mid.yaml` declaring `source: ./local-src` looked for `<root>/local-src`, missed it, fell through to the remote path, and go-getter turned the relative path into `https://./local-src` — vendoring failed outright. Record the declaring manifest per source while manifests are read, and anchor relative local sources to it, so a manifest stays self-contained no matter which manifest imports it or how deeply. Provenance is recorded in mergeVendorConfigFiles rather than processVendorImports because one import can name a directory of manifests, and each file in it anchors its own sources. determineSourceType resolved the anchored path but rebound its local pointer instead of writing through, so the caller kept the relative URI and the download resolved it against the process working directory. Write it back, and return before url.Parse now that the URI is a filesystem path — parsing it as a URL would read a Windows drive letter as a scheme. Targets are deliberately left anchored to one root for every source, imported or not: they describe where artifacts land in the project, so splitting a manifest into imports must not move them. Renames processTargetsParams.VendorConfigFilePath to SourceBasePath so it reads as the pair to TargetBasePath.
… into claude/github-issue-2409-by4npj
This PR pushed internal/exec/vendor_utils.go from 589 to 673 lines, past the 600-line limit in CLAUDE.md, and the file is not on the revive file-length-limit exception list. Move the three path-resolution questions into vendor_paths.go — where the manifest is, where a relative source points, where a relative target lands — and document at the top why sources and targets resolve differently. Pure move: the only change to vendor_utils.go is dropping the now-unused net/url import. vendor_utils.go is back to 550 lines, below where this PR found it. Also drop the loop in processVendorImports that overwrote each imported source's File with the raw import path. Sources now record the manifest they were read from in mergeVendorConfigFiles, which is more precise when an import names a directory of manifests, and no longer clobbers a `file` a source declares itself. Error messages are unchanged for the common case, where the import path and the resolved manifest path are the same string.
|
thejrose1984 please review the failed tests https://github.com/cloudposse/atmos/actions/runs/30705193627/job/91399842837?pr=2833 |
TestCopyFile_FailCreate makes the destination directory read-only and expects os.Create to fail. Root bypasses the permission check, so os.Create succeeds and the test fails for anyone running the suite as root — in a root container or devcontainer, the failure is unconditional and unrelated to their changes. Guard it with the same root check the rest of the repo uses for permission tests (pkg/config/load_error_paths_unix_test.go, terraform_generate_varfile_unix_test.go). No coverage is lost: TestCopyFile_CreateDestFileError exercises the same create-failure path by putting a directory in the destination's place, which fails regardless of privileges.
… into claude/github-issue-2409-by4npj
Five tests fail for reasons unrelated to the code under test, so a local run reports failures nobody introduced: - TestWriteEnvToFile_ErrorCases/fails_with_read-only_directory, TestProcessChdirFlag/directory_with_no_permissions, TestCopyFile_FailCreate (already guarded in the previous commit) and TestCollectDirEntries_PropagatesGenericWalkError expect a permission denial that root does not get. Guard them with the root check already used in pkg/config/load_error_paths_unix_test.go and elsewhere; three of the four already had the matching Windows guard beside it. - TestMainHooksAndStoreIntegration and TestMainTerraformPlanDiffIntegration shell out to terraform through main() and fail with "executable file not found in $PATH" when it isn't installed. Gate them on tests.RequireTerraform, the precondition helper the rest of the suite uses for this. All now skip with a reason instead of failing, which is what the testing strategy in docs/prd/testing-strategy.md asks for.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/env/env_test.go`:
- Around line 356-360: Replace direct os.Getuid() calls in cmd/env/env_test.go
lines 356-360 and cmd/root_process_chdir_test.go lines 108-111 with a shared
OS-specific root-detection helper. Implement the helper to use Unix root
detection on supported platforms and return false on Windows, then keep the
existing skip behavior based on that helper.
🪄 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 Plus
Run ID: a82f89a6-b4a9-47f8-8758-986000b8206b
📒 Files selected for processing (2)
cmd/env/env_test.gocmd/root_process_chdir_test.go
| // Root bypasses the read-only directory this case relies on, so the write would succeed. | ||
| if os.Getuid() == 0 { | ||
| t.Skip("Skipping permission test when running as root") | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)cmd/(env/env_test\.go|root_process_chdir_test\.go)$|(^|/)cmd/.*/_test\.go$' | head -200
echo
echo "== occurrences of os.Getuid =="
rg -n '\bos\.Getuid\(' .
echo
echo "== occurrences of uid helper patterns =="
rg -n 'IsValid(os\.Geteuid|uid|root|nonroot|OS=windows|GOOS=windows)' .
echo
echo "== relevant snippets =="
sed -n '340,370p' cmd/env/env_test.go
printf '\n---\n'
sed -n '90,120p' cmd/root_process_chdir_test.go
echo
echo "== Go runtime availability and os.Getuid behavior docs from local Go if present =="
if command -v go >/dev/null 2>&1; then
go version
go doc os.Getuid 2>&1 || true
tmp="$(mktemp -d)"
cat > "$tmp/check.go" <<'GO'
package main
import (
"fmt"
"os"
)
func main() {
fmt.Println("goos", os.Getenv("GOOS"))
fmt.Println("uid", os.Getuid())
}
GO
GOOS=windows GOARCH=amd64 go build -o "$tmp/check.exe" "$tmp/check.go" 2>&1 || true
rm -rf "$tmp"
else
echo "go not available"
fiRepository: cloudposse/atmos
Length of output: 1499
Use build-safe root detection in the Windows-unaware tests.
Both cmd/env/env_test.go#L356-L360 and cmd/root_process_chdir_test.go#L108-L111 call the Unix-only os.Getuid() API. Move this check behind an OS-specific helper so Windows test builds are not blocked; the Windows implementation must return false.
📍 Affects 2 files
cmd/env/env_test.go#L356-L360(this comment)cmd/root_process_chdir_test.go#L108-L111
🤖 Prompt for 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.
In `@cmd/env/env_test.go` around lines 356 - 360, Replace direct os.Getuid() calls
in cmd/env/env_test.go lines 356-360 and cmd/root_process_chdir_test.go lines
108-111 with a shared OS-specific root-detection helper. Implement the helper to
use Unix root detection on supported platforms and return false on Windows, then
keep the existing skip behavior based on that helper.
Source: Coding guidelines
|
💥 This pull request now has conflicts. Could you fix it thejrose1984? 🙏 |
Reconcile the vendor base-path anchoring in this PR with main's vendor refactor (per-target semver-range version overrides + the install package): - processTargetsParams / vendorSourceParams keep main's version-override fields (install.PkgType, RawVersion, RefreshLock, Lister) alongside this PR's split anchors (SourceBasePath for local sources, TargetBasePath for targets), replacing the single VendorConfigFilePath anchor. - resolveAtmosVendorSource and resolveTargetOverride classify sources against the declaring manifest (resolveVendorSourceBasePath); processTargets resolves targets with resolveVendorTargetPath (absolute targets honored) and keeps main's filepath.Abs for the lock containment check. - determineSourceType / resolveVendorConfigFilePath now live only in vendor_paths.go; main's copies (including the uri=&absPath bug this PR fixes) are dropped. - Test literals updated to the split-anchor fields and install.PkgType*, and the target-path assertions account for the absolute paths processTargets now returns.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/exec/vendor_paths.go (1)
113-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the package error contract for source parsing failures.
Wrap both failures with a static error from
errors/errors.goand add source URI context. Line 116 creates a dynamic error. Line 132 returns the parser error without context. This prevents callers from classifying the error consistently.As per coding guidelines, “Wrap all errors with static errors from
errors/errors.go” and “never use dynamic errors directly.”🤖 Prompt for 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. In `@internal/exec/vendor_paths.go` around lines 113 - 132, The source parsing failure paths in the vendor URI handling must use the package’s static errors from errors/errors.go and include the offending URI as context. Update the error returned after JoinPathAndValidate and the url.Parse failure in the surrounding source-parsing flow, reusing the appropriate predefined error symbols and wrapping the underlying errors rather than returning dynamic or parser errors directly.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@internal/exec/vendor_paths.go`:
- Around line 113-132: The source parsing failure paths in the vendor URI
handling must use the package’s static errors from errors/errors.go and include
the offending URI as context. Update the error returned after
JoinPathAndValidate and the url.Parse failure in the surrounding source-parsing
flow, reusing the appropriate predefined error symbols and wrapping the
underlying errors rather than returning dynamic or parser errors directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35dffb60-6948-4a09-945b-536bf824899b
📒 Files selected for processing (15)
cmd/env/env_test.gocmd/root_process_chdir_test.gointernal/exec/vendor_paths.gointernal/exec/vendor_pull_integration_test.gointernal/exec/vendor_utils.gointernal/exec/vendor_utils_test.gomain_hooks_and_store_integration_test.gomain_plan_diff_integration_test.gopkg/archive/archive_test.gopkg/schema/schema.gopkg/vendoring/install/copy_glob_unix_test.gowebsite/docs/cli/commands/vendor/vendor-pull.mdxwebsite/docs/cli/configuration/vendor.mdxwebsite/docs/vendor/config/imports.mdxwebsite/docs/vendor/config/sources.mdx
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/root_process_chdir_test.go
- pkg/schema/schema.go
- website/docs/cli/commands/vendor/vendor-pull.mdx
- website/docs/cli/configuration/vendor.mdx
- cmd/env/env_test.go
what
targetsin a vendor manifest now resolve against the Atmosbase_pathwhenatmos.yamldeclares the manifest's location viavendor.base_path. A manifest discovered in the working directory (novendor.base_pathconfigured) keeps resolving targets relative to itself, so the common single-directory layout is unchanged.targetsare used as-is.filepath.Joinwas nesting them under the base path, so/opt/xbecameopt/x.sourcedeclared in an imported manifest now resolves against that manifest's directory instead of the root manifest's. Each source records the manifest that declared it as it's read, and local sources anchor to it — through any depth of nested imports.determineSourceTypenow writes the anchored path back to its caller. It computed the path but rebound a local pointer (uri = &absPath), so the URI the package was actually downloaded from stayed relative and resolved against the process working directory.targetsandsourceresolution rules inwebsite/docs/vendor/config/sources.mdx, added a "Relative paths in imported manifests" section toimports.mdx, and documented thevendor.base_pathinteraction incli/configuration/vendor.mdxandcli/commands/vendor/vendor-pull.mdx.Deliberately not changed:
targetsstay anchored to a single root for every source, imported or not. They describe where artifacts land in the project, so splitting a manifest into imports must not move them. The asymmetry withsourceis now documented rather than implicit.why
atmos.yamlandvendor.yamltogether in a config directory dropped the vendoredcomponents/terraformtree next to the manifest instead of underbase_path, wherecomponents.terraform.base_pathandstacks.base_pathpoint.ExecuteAtmosVendorInternaljoined every relative target tofilepath.Dir(vendorConfigFileName).vendor.yamlfile is detected by Atmos using thebase_pathsetting inatmos.yaml, thetargetspaths will be considered relative to thebase_path" — the code never implemented it.docs/prd/base-path-resolution-semantics.md, whose core convention is that a relative path in a configuration file is anchored to the file declaring it.processVendorImportsrecorded provenance, but a single anchor built from the root manifest was handed to every merged source. The failure was not subtle: a manifest atvendor/mid.yamldeclaringsource: ./local-srclooked for<root>/local-src, missed it, fell through to the remote path, and go-getter turned the relative path intohttps://./local-src. Vendoring failed outright.mergeVendorConfigFilesrather thanprocessVendorImportsbecause one import can name a directory of manifests, and each file in it anchors its own sources.Testing
atmos vendor pullagainst generated projects: the config-directory layout from Vendored artefacts are targeted relative to the vendor.yaml file, not the base_path specified in atmos.yaml #2409, the no-vendor.base_pathfallback, and a chain of nested imports where three manifests at three depths each vendor a siblinglocal-srcwhose marker file identifies it — so anchoring to the wrong manifest cannot accidentally pass. Each was confirmed to fail before the fix.vendor-simple-excludes --tags=simple, and thevendorfixture's--tags=dir(path-traversal source) and--tags=file(thefile://form, whose separate branch is untouched) all produce exactly the filestests/test-cases/vendor-test.yamlexpects.GOOS=windows go vetis clean, and absolute-path tests derive fromt.TempDir()— a separator-rooted path like\opt\xis not absolute on Windows, so those assertions would otherwise have exercised the relative branch there.references
docs/prd/base-path-resolution-semantics.md— the relative-path convention the source anchoring now followsSummary by CodeRabbit
Bug Fixes
vendor pullpath resolution for relative targets using the configured Atmosbase_path.Documentation
base_path,vendor.base_path, and imported manifests.