Repository navigation
fix: resolve !include failure in describe affected for base-ref stacks - #2100
Merged
Merged
Conversation
…cks (#2090) Save and restore `BasePath` and `BasePathAbsolute` in `executeDescribeAffected()` when switching context to process base-ref stacks. Also update `findLocalFile` to prefer `BasePathAbsolute` over `BasePath` for reliable path resolution. Add unit tests, integration tests with fixture, cross-platform path handling, and fix doc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
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:
📝 WalkthroughWalkthroughPreserve and restore BasePath/BasePathAbsolute during base-ref (remote) stack processing and prefer BasePathAbsolute when resolving local !include files; add unit and integration tests, fixtures, and docs clarifying ordered include path-resolution behavior. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (describe affected)
participant DA as DescribeAffected Processor
participant YAML as YAML !include Resolver
participant FS as Local FS
participant Remote as Base-ref Repo
CLI->>DA: ExecuteDescribeAffected (HEAD, BASE)
DA->>DA: capture BasePath / BasePathAbsolute
DA->>Remote: compute repo-root-relative remap & resolve remote stack paths
Remote-->>DA: remote stack manifest path
DA->>YAML: request include resolution (passes BasePathAbsolute)
YAML->>FS: findLocalFile (prefer BasePathAbsolute → manifest-relative → BasePath)
alt file found
FS-->>YAML: file contents
YAML-->>DA: resolved include
else not found
FS-->>YAML: not found
YAML-->>DA: error (message uses BasePathAbsolute if available)
end
DA->>DA: restore original BasePath / BasePathAbsolute
DA-->>CLI: results (affected components / errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/utils/yaml_include_by_extension.go (1)
202-209: Fix is correct — path resolution now usesBasePathAbsolutereliably.The core change is solid. One minor note: the "prefer
BasePathAbsolute, fallback toBasePath" pattern is now identical across lines 102–105 (error path) and 204–207 (resolution path). Could be DRYed up with a small helper.♻️ Optional: extract the repeated fallback into a helper
Add a private helper (same file, no new public API):
// effectiveBasePath returns BasePathAbsolute when set, falling back to BasePath. func effectiveBasePath(cfg *schema.AtmosConfiguration) string { if cfg.BasePathAbsolute != "" { return cfg.BasePathAbsolute } return cfg.BasePath }Then replace both blocks:
- errBasePath := atmosConfig.BasePathAbsolute - if errBasePath == "" { - errBasePath = atmosConfig.BasePath - } + errBasePath := effectiveBasePath(atmosConfig) return fmt.Errorf("%w: could not find local file '%s' (tried relative to manifest '%s' and base path '%s')", ErrIncludeYamlFunctionInvalidFile, includeFile, file, errBasePath)- basePath := atmosConfig.BasePathAbsolute - if basePath == "" { - basePath = atmosConfig.BasePath - } + basePath := effectiveBasePath(atmosConfig) atmosManifestPath := filepath.Join(basePath, includeFile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/yaml_include_by_extension.go` around lines 202 - 209, The repeated fallback logic choosing atmosConfig.BasePathAbsolute over atmosConfig.BasePath should be extracted into a small private helper to avoid duplication; add a helper like effectiveBasePath(cfg *schema.AtmosConfiguration) string that returns cfg.BasePathAbsolute when non-empty else cfg.BasePath, then replace both occurrences where you compute basePath (the block that sets basePath := atmosConfig.BasePathAbsolute / fallback to BasePath and the equivalent error-path block) to call effectiveBasePath(atmosConfig) before joining with includeFile and calling resolveAbsolutePath.tests/describe_affected_include_test.go (1)
176-208:AtmosConfigis not passed toExecuteDescribeComponentParams.
InitCliConfigresult is discarded (_), andExecuteDescribeComponentParamsdoesn't setAtmosConfig. This works becauseInitCliConfigsets global state, but it's worth noting the implicit coupling. Same pattern is used inTestDescribeAffectedWithIncludeVerifyIncludedValues(line 215).Not blocking — just flagging the implicit global dependency for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/describe_affected_include_test.go` around lines 176 - 208, The test discards the InitCliConfig return and never sets AtmosConfig on ExecuteDescribeComponentParams, relying on implicit global state; update TestDescribeAffectedWithIncludeComponentsLoadCorrectly (and similarly TestDescribeAffectedWithIncludeVerifyIncludedValues) to capture the config returned by cfg.InitCliConfig and pass it into e.ExecuteDescribeComponent by populating the AtmosConfig field of e.ExecuteDescribeComponentParams so the call uses the explicit AtmosConfig instead of implicit globals.internal/exec/describe_affected_utils.go (1)
82-85: Consider wrapping thefilepath.Relerror with context.Consistent with the existing bare returns on lines 86-101, but the coding guidelines ask for wrapped errors. Low priority since it matches the pre-existing style here.
♻️ Optional: wrap error for easier debugging
baseRelPath, err := filepath.Rel(localRepoFileSystemPathAbs, currentBasePathAbsolute) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, fmt.Errorf("computing base path relative to repo root: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/describe_affected_utils.go` around lines 82 - 85, The call to filepath.Rel that computes baseRelPath should wrap the returned error with context before returning; replace the bare return of err in the block that computes baseRelPath (which uses localRepoFileSystemPathAbs and currentBasePathAbsolute) with a wrapped error such as using fmt.Errorf("calculating relative path from %s to %s: %w", localRepoFileSystemPathAbs, currentBasePathAbsolute, err) so callers get useful debug info while preserving the original error via %w.pkg/utils/yaml_include_by_extension_test.go (1)
371-381: Preferrequire.NoErrorfor setup steps where subsequent code depends on success.If
os.MkdirAlloros.WriteFilefails, the test continues with misleading failures fromfindLocalFile. This matches the pre-existing pattern in the file, so it's optional — butrequirewould be cleaner for setup operations.♻️ Example for the setup block
- err := os.MkdirAll(policyDir, 0o755) - assert.NoError(t, err) + err := os.MkdirAll(policyDir, 0o755) + require.NoError(t, err) policyFile1 := filepath.Join(policyDir, "notification-failure.rego") - err = os.WriteFile(policyFile1, []byte("package notification\n"), 0o644) - assert.NoError(t, err) + err = os.WriteFile(policyFile1, []byte("package notification\n"), 0o644) + require.NoError(t, err)Same applies to setup blocks in
TestIncludeMultipleInSameFileandTestIncludeBasePathResolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/yaml_include_by_extension_test.go` around lines 371 - 381, Replace assert.NoError calls used in setup steps with require.NoError so failures abort the test immediately: change the os.MkdirAll(policyDir, 0o755) check and the os.WriteFile(policyFile1, ...) / os.WriteFile(policyFile2, ...) checks in the setup block to use require.NoError(t, err) instead of assert.NoError(t, err); apply the same change to the equivalent setup blocks in TestIncludeMultipleInSameFile and TestIncludeBasePathResolution to ensure subsequent calls like findLocalFile don't run on failed setup.
🤖 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/describe_affected_utils.go`:
- Around line 82-85: The call to filepath.Rel that computes baseRelPath should
wrap the returned error with context before returning; replace the bare return
of err in the block that computes baseRelPath (which uses
localRepoFileSystemPathAbs and currentBasePathAbsolute) with a wrapped error
such as using fmt.Errorf("calculating relative path from %s to %s: %w",
localRepoFileSystemPathAbs, currentBasePathAbsolute, err) so callers get useful
debug info while preserving the original error via %w.
In `@pkg/utils/yaml_include_by_extension_test.go`:
- Around line 371-381: Replace assert.NoError calls used in setup steps with
require.NoError so failures abort the test immediately: change the
os.MkdirAll(policyDir, 0o755) check and the os.WriteFile(policyFile1, ...) /
os.WriteFile(policyFile2, ...) checks in the setup block to use
require.NoError(t, err) instead of assert.NoError(t, err); apply the same change
to the equivalent setup blocks in TestIncludeMultipleInSameFile and
TestIncludeBasePathResolution to ensure subsequent calls like findLocalFile
don't run on failed setup.
In `@pkg/utils/yaml_include_by_extension.go`:
- Around line 202-209: The repeated fallback logic choosing
atmosConfig.BasePathAbsolute over atmosConfig.BasePath should be extracted into
a small private helper to avoid duplication; add a helper like
effectiveBasePath(cfg *schema.AtmosConfiguration) string that returns
cfg.BasePathAbsolute when non-empty else cfg.BasePath, then replace both
occurrences where you compute basePath (the block that sets basePath :=
atmosConfig.BasePathAbsolute / fallback to BasePath and the equivalent
error-path block) to call effectiveBasePath(atmosConfig) before joining with
includeFile and calling resolveAbsolutePath.
In `@tests/describe_affected_include_test.go`:
- Around line 176-208: The test discards the InitCliConfig return and never sets
AtmosConfig on ExecuteDescribeComponentParams, relying on implicit global state;
update TestDescribeAffectedWithIncludeComponentsLoadCorrectly (and similarly
TestDescribeAffectedWithIncludeVerifyIncludedValues) to capture the config
returned by cfg.InitCliConfig and pass it into e.ExecuteDescribeComponent by
populating the AtmosConfig field of e.ExecuteDescribeComponentParams so the call
uses the explicit AtmosConfig instead of implicit globals.
Components 5-9 were added to the shared nonprod.yaml fixture, breaking the TestUnmarshalYAMLFromFile golden test that expects only components 1-4. Move them to a dedicated extended.yaml stack file with its own stage mixin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
February 23, 2026 05:46
52ec813
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
==========================================
+ Coverage 76.48% 76.49% +0.01%
==========================================
Files 831 831
Lines 79072 79088 +16
==========================================
+ Hits 60476 60501 +25
+ Misses 14827 14815 -12
- Partials 3769 3772 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Andriy Knysh (aknysh)
requested a review
from Erik Osterman (Cloud Posse) (osterman)
February 23, 2026 14:44
Expand the Local Sources section in !include and !include.raw docs to explain exactly what each path type is relative to, the resolution order (absolute → manifest-relative → base_path-relative), and when each strategy applies (./.. prefix vs bare paths). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
February 23, 2026 15:49
20c79b2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
website/docs/functions/yaml/include.mdx (1)
99-116: Strategy 3 + Note create a subtle contradiction.Strategy 3 (lines 99–107) defines itself as covering "all other relative paths" and says they resolve "relative to
base_path". The Note then clarifies those same bare paths first try manifest-relative resolution (strategy 2), and only fall back tobase_path. This makes strategies 2 and 3 appear mutually exclusive by prefix, but the Note reveals they're not.A reader skimming only the numbered list gets the wrong mental model. Consider absorbing the first-match-wins caveat directly into strategy 3 to eliminate the contradiction:
📝 Suggested restructure
-3. **Paths relative to `base_path`** — All other relative paths (those that do **not** start with `./` or `../`) are - resolved relative to the [`base_path`](/cli/configuration#base-path) setting in `atmos.yaml`. This is the most - common pattern when referencing shared configuration files from any stack manifest, regardless of the manifest's - location in the directory tree. +3. **Paths relative to `base_path`** — All other relative paths (those that do **not** start with `./` or `../`) + are resolved with **first-match-wins**: Atmos first checks relative to the manifest directory, and if not found, + falls back to the [`base_path`](/cli/configuration#base-path) in `atmos.yaml`. Use bare paths (no prefix) for + `base_path`-relative resolution; use `./` or `../` to make manifest-relative resolution unambiguous. ```yaml # Resolved relative to `base_path` in atmos.yaml (e.g., the repo root) vars: !include stacks/catalog/vpc/vars.yaml policy: !include config/policies/deny.rego ```Then the separate Note (lines 109–116) can be simplified or removed to avoid duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/functions/yaml/include.mdx` around lines 99 - 116, Strategy 3 currently states that "all other relative paths" resolve to base_path but the separate Note contradicts this by saying bare paths first try manifest-relative then base_path; update the Strategy 3 paragraph to include the first-match-wins caveat explicitly (e.g., "Bare paths are resolved by first checking the manifest directory, then falling back to base_path; the first match wins") and keep the example YAML block as-is, then remove or simplify the separate Note so the behavior is not duplicated or contradicted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/docs/functions/yaml/include.raw.mdx`:
- Around line 196-197: Step 3 in include.raw.mdx currently says bare paths
“resolve relative to the [`base_path`]” but this oversimplifies behavior; update
the Step 3 description to state that bare (non-absolute) paths are resolved with
a first-match-wins order: first try manifest-relative (relative to the file
containing the include), and if not found then fall back to [`base_path`], and
keep the example `!include.raw stacks/catalog/template.tf` to illustrate this;
reference include.mdx’s Note behavior and ensure the wording mentions the
first-try manifest-relative then base_path fallback.
---
Nitpick comments:
In `@website/docs/functions/yaml/include.mdx`:
- Around line 99-116: Strategy 3 currently states that "all other relative
paths" resolve to base_path but the separate Note contradicts this by saying
bare paths first try manifest-relative then base_path; update the Strategy 3
paragraph to include the first-match-wins caveat explicitly (e.g., "Bare paths
are resolved by first checking the manifest directory, then falling back to
base_path; the first match wins") and keep the example YAML block as-is, then
remove or simplify the separate Note so the behavior is not duplicated or
contradicted.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/fixes/2026-02-22-describe-affected-include-basepath.mdwebsite/docs/functions/yaml/include.mdxwebsite/docs/functions/yaml/include.raw.mdx
Absorb the first-match-wins caveat into strategy 3 description in both include.mdx and include.raw.mdx so bare paths clearly document that they first try manifest-relative, then fall back to base_path-relative. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/docs/functions/yaml/include.mdx (1)
99-107: Minor: example comment at Line 104 contradicts the first-match-wins prose above it.The prose on lines 100–101 says bare paths check manifest-relative first, then fall back to
base_path. But the comment on line 104 says "Resolved relative tobase_pathin atmos.yaml", implying the lookup always goes straight tobase_path. A user reading only the code block will get the wrong mental model.📝 Align the comment with the first-match-wins strategy
- # Resolved relative to `base_path` in atmos.yaml (e.g., the repo root) + # First checks manifest directory; if not found, falls back to `base_path` in atmos.yaml vars: !include stacks/catalog/vpc/vars.yaml policy: !include config/policies/deny.rego🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/docs/functions/yaml/include.mdx` around lines 99 - 107, The inline YAML comment in the example block (the comment that currently says "Resolved relative to `base_path` in atmos.yaml") contradicts the documented "first-match-wins" behavior; update that comment to state that bare paths are resolved first relative to the manifest directory and, if not found, fall back to the `base_path` from atmos.yaml so it matches the prose—apply this change to the example lines containing the vars: !include stacks/catalog/vpc/vars.yaml and policy: !include config/policies/deny.rego entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/docs/functions/yaml/include.mdx`:
- Around line 99-107: The inline YAML comment in the example block (the comment
that currently says "Resolved relative to `base_path` in atmos.yaml")
contradicts the documented "first-match-wins" behavior; update that comment to
state that bare paths are resolved first relative to the manifest directory and,
if not found, fall back to the `base_path` from atmos.yaml so it matches the
prose—apply this change to the example lines containing the vars: !include
stacks/catalog/vpc/vars.yaml and policy: !include config/policies/deny.rego
entries.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
website/docs/functions/yaml/include.mdxwebsite/docs/functions/yaml/include.raw.mdx
Apply reviewer suggestion: replace inline prose with numbered sub-list showing resolution order (1. manifest directory, 2. base_path) for bare paths in both !include and !include.raw docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrite strategies 2 and 3 in both include.mdx and include.raw.mdx to accurately reflect the code: ./.. paths resolve relative to manifest directory, bare paths resolve relative to base_path. Expand include.raw local files section to be fully self-contained instead of cross-referencing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
February 25, 2026 18:20
4062a41
Andriy Knysh (aknysh)
requested a review
from Erik Osterman (Cloud Posse) (osterman)
February 25, 2026 18:22
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
February 26, 2026 19:58
6e28012
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
February 26, 2026 20:03
ce1940e
Andriy Knysh (aknysh)
requested a review
from Erik Osterman (Cloud Posse) (osterman)
February 26, 2026 20:04
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Feb 26, 2026
|
These changes were released in v1.208.0. |
|
These changes were released in v1.208.1-test.9. |
|
These changes were released in v1.208.1-test.10. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
!includeYAML function failing whenatmos describe affectedprocesses base-ref stacksBasePathandBasePathAbsoluteinexecuteDescribeAffected()when switching context to the remote repofindLocalFileto preferBasePathAbsoluteoverBasePathfor reliable path resolution!includewithdescribe affected!includeintegration test coverage for!include.raw,.txt,.tf, extensionless files, and advanced YQ expressionswhy
atmos describe affectedcompares HEAD and BASE stacks by temporarily pointingatmosConfigpaths to the base-ref checkout. It saved/restored 5 path fields but missedBasePathandBasePathAbsolute, causing!includeto fail when resolving files relative to the base path in the remote repofindLocalFileusedBasePath(which can be a relative path like"./") instead ofBasePathAbsolute, causing resolution failures when the CWD differs from the repo root!includein a file could succeed (file exists in CWD) while the second fails (file only exists in the PR branch), making the bug appear intermittentreferences
atmos describe affectedfails on!includefor one file and not the other #2090docs/fixes/2026-02-22-describe-affected-include-basepath.mdSummary by CodeRabbit
Bug Fixes
Tests
Documentation