Repository navigation
feat(list): --process-templates and --process-functions flags; fix list instances --upload auth - #2363
Merged
Conversation
…omponent `atmos list instances --upload` was failing in CI with "No valid credential sources found" when component sections contain `atmos.Component(...)` Go-template calls. The processor gated per-component auth on `processYamlFunctions`, but `atmos.Component(...)` is a Go template function, not a YAML function; it runs during the template branch and also consumes `info.AuthContext` to authenticate terraform subprocesses against remote backends. Widen the guard to `processYamlFunctions || processTemplates` so list instances (and any future caller with the same shape) gets credentials for the per-component auth path. Refactor the inline block into `resolveComponentAuthManager` with an injectable resolver for testability; add `shouldResolvePerComponentAuth` as a named predicate so the fix is self-documenting. Category A callers (terraform/helmfile/describe-component) pass `processYamlFunctions=true` and take the unchanged branch. The only observable behavior change is for callers with `processTemplates=true, processYamlFunctions=false` — today that is `list instances` and its provenance-tree branch, which were broken. See docs/fixes/2026-04-24-list-instances-per-component-auth.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ommands Add `--process-templates` / `--process-functions` flags (and their ATMOS_PROCESS_TEMPLATES / ATMOS_PROCESS_FUNCTIONS env var bindings) to every `atmos list` subcommand that processes stack configurations, for parity with `atmos describe affected` / `atmos describe stacks` / `atmos describe component`. Both flags default to true. Commands updated: - `atmos list instances` (new) - `atmos list components` (new) - `atmos list metadata` (new) - `atmos list sources` (new) - `atmos list stacks` (new) Already had the flags: `list affected`, `list settings`, `list values` (and its `list vars` alias). Skipped: `list aliases`, `list themes`, `list vendor`, `list workflows` — these do not process stacks or use them only internally for enumeration where the flags would be no-ops. Also: - Clarify `WithProcessFunctionsFlag` / `WithProcessTemplatesFlag` wrapper descriptions to distinguish YAML functions (`!terraform.state`, `!terraform.output`, `!store`, `!aws.*`) from Go template functions (`atmos.Component(...)`) — they are controlled by separate flags. - Thread the two flags through `InstancesCommandOptions` / `MetadataOptions` in `pkg/list/` and through both the matrix-format and tree-format branches of `list_instances.go` so output is consistent with the non-matrix, non-tree path for the same invocation. - Clarify the misleading comment in `processInstancesWithDeps` that conflated the YAML-function flag with `atmos.Component(...)` (which is a Go template function). - Update the `list instances` docs page with the two new flags. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the fix doc to cover: - The refactor that extracted the guard into the named helper `shouldResolvePerComponentAuth` and the inline block into `resolveComponentAuthManager` with an injectable `componentAuthManagerResolver` for test doubles. - The three-layer regression-test suite (`TestShouldResolvePerComponentAuth`, `TestResolveComponentAuthManager` six-row table, `TestResolveComponentAuthManager_ResolverErrorFallsBackToParent`). - The companion `--process-templates` / `--process-functions` flag rollout across the `atmos list` surface (instances, components, metadata, sources, stacks), the matrix-/tree-format plumbing, and the reworded flag help that disambiguates YAML functions from Go template functions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Docs: add the two flags to every `atmos list` subcommand page that now exposes them but was missing the entry: list-components, list-metadata, list-settings, list-sources, list-stacks, list-values, list-vars. Each entry names the default (true), disambiguates Go template functions (`atmos.Component(...)`) from YAML functions (`!terraform.state`, `!terraform.output`, `!store`, `!aws.*`), and links the matching `ATMOS_PROCESS_TEMPLATES` / `ATMOS_PROCESS_FUNCTIONS` env var. Tests: three layers of regression coverage for each command that just got the flags (instances, components, metadata, sources, stacks): - Parser wiring — `Test*ProcessTemplatesAndFunctionsFlags` queries the real `<command>Cmd.Flags()` and asserts both flags are registered with default "true". Guards against future removal of `WithProcessTemplatesFlag` / `WithProcessFunctionsFlag` from the command's `NewListParser(...)` call. - Options struct — `Test*Options_ProcessTemplatesAndFunctions` over all four (templates, functions) combinations; guards against field rename/removal from the cmd- and pkg-level option structs. - Flag propagation — `TestProcessInstancesWithDeps_PropagatesTemplate AndFunctionFlags` uses a gomock mock of `e.StacksProcessor` and asserts the `(processTemplates, processYamlFunctions)` pair reaches `ExecuteDescribeStacks` unchanged across all four quadrants. Regression guard for the original hardcoded-`(true, false)` shape the `list instances --upload` hang was about. Also backfills a `cmd/list/metadata_test.go` for the previously-untested metadata command and extends the existing `TestMetadataOptionsStruct` to cover the two new fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New blog post covers the two flags added to every `atmos list` subcommand that processes stack manifests (instances, components, metadata, sources, stacks), the YAML-function vs. Go-template-function distinction the flag help used to conflate, and the escape-hatch patterns for environments without `tofu` / `terraform` on $PATH. Briefly references the underlying per-component auth fix that made defaulting both flags to `true` safe for the `list instances --upload` path. Also adds the matching milestone to the `discoverability` initiative in the roadmap data file. Progress stays at 100% (9/9 shipped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ran `go get -u ./... && go mod tidy`. Updates land on everything that's compatible; three transitive pins remain in place and are documented inline in go.mod so the next `-u` pass does not re-bump them blindly: - `getsentry/sentry-go` — held at v0.45.1 because `cockroachdb/errors v1.12.0` (the latest) still references `sentry.Event.Extra`, which was removed in sentry-go v0.46.0. - `gocloud.dev` (indirect) — held at v0.41.0 because `hairyhenderson/gomplate/v3`'s s3blob code uses `s3blob.URLOpener.ConfigProvider`, which was removed in gocloud.dev v0.42+. - `hairyhenderson/go-fsimpl` (indirect) — held at v0.3.1 because newer go-fsimpl versions require the gocloud.dev bump above. Notable direct-dep bumps: aws-sdk-go-v2/service/s3 1.99.1→1.100.0, smithy-go 1.25.0→1.25.1, anthropic-sdk-go 1.37.0→1.38.0, hashicorp/terraform-exec 0.25.0→0.25.1, posthog-go 1.11.3→1.12.1, k8s.io/client-go 0.35.4→0.36.0. `go build ./...` and `go vet ./...` are clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit fixes on the list-flag rollout: - `pkg/list/list_instances.go` — tree-format branch now honors the caller-supplied `ProcessTemplates` / `ProcessFunctions` flags, matching the behavior of `cmd/list/stacks.go` tree format and what the fix doc claims. Previously the tree path still had hardcoded `(false, false)` after the flag rollout. - `website/docs/cli/commands/list/list-instances.mdx` — normalize the env-var note to `<br/>Environment variable: ATMOS_*`, matching the seven other list-*.mdx files. Unrelated housekeeping: - Bump `ATMOS_VERSION` references from 1.216.0 to 1.217.0 in the quick-start-advanced Dockerfile and in two tests that assert against the version string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
|
🚀 Go Version Change Detected This PR changes the Go version:
Tip Upgrade Checklist
This is an automated comment from the Go Version Check action. |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
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 |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds two flags—--process-templates and --process-functions—to list subcommands, threads them through list/describe execution paths, refactors per-component auth resolution to run when either mode is enabled with an injectable resolver, and updates tests, docs, and dependency/version metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (atmos list ...)
participant Cmd as List Command
participant ListPkg as pkg/list.ExecuteList*
participant Describe as ExecuteDescribeStacks
participant CompProc as ComponentProcessor
participant AuthRes as componentAuthResolver
participant AuthMgr as AuthManager
CLI->>Cmd: parse --process-templates / --process-functions
Cmd->>ListPkg: call ExecuteList* with flags
ListPkg->>Describe: ExecuteDescribeStacks(processTemplates, processFunctions)
Describe->>CompProc: process component entries
CompProc->>AuthRes: shouldResolvePerComponentAuth?(templates || functions)
AuthRes-->>CompProc: return component-level or parent AuthManager
CompProc->>AuthMgr: use returned AuthManager for per-component ops
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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 docstrings
🧪 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 |
…into aknysh/update-upload-instances
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NOTICE (1)
1-1830:⚠️ Potential issue | 🟠 MajorRegenerate
NOTICEfrom source-of-truth to unblock CI.The dependency-review job already reports this file is out of date; since
NOTICEis generated, please rerun./scripts/generate-notice.shand commit the result instead of manually curating entries.Based on learnings: In cloudposse/atmos, the NOTICE file is programmatically generated and should not be manually edited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NOTICE` around lines 1 - 1830, The NOTICE file was manually edited and is out-of-date; regenerate it from the source-of-truth by running the provided generator and commit the generated output: run ./scripts/generate-notice.sh, replace the current NOTICE with the script's output, verify CI passes, and commit the regenerated NOTICE (do not hand-edit the NOTICE file).
🧹 Nitpick comments (7)
pkg/list/list_metadata_test.go (1)
359-406: These combo tests should target behavior, not field assignment.Both tests currently re-check values assigned in the test itself. Consider replacing with assertions that these options actually affect metadata/instances execution behavior.
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 `@pkg/list/list_metadata_test.go` around lines 359 - 406, The tests TestMetadataOptions_ProcessTemplatesAndFunctionsAllCombinations and TestInstancesCommandOptions_ProcessTemplatesAndFunctions are tautological because they only re-assert fields set in the test; change them to assert behavior: construct a MetadataOptions or InstancesCommandOptions with each flag combo, call the real processing function(s) that use those structs (e.g., the metadata/instance processing pipeline functions that accept MetadataOptions or InstancesCommandOptions), and assert side-effects for each flag (for example, when ProcessTemplates is true templates are expanded/replaced and when false they remain intact; when ProcessFunctions is true function calls are executed and when false they are not). If needed, inject test doubles or hooks into the processing functions to observe whether template or function handlers ran (use DI or a small fake processor), and replace the current field-only assertions with checks against the processed output or observed handler invocations for each combo.pkg/list/list_instances_coverage_test.go (1)
44-59: Strengthen this wrapper test to validate behavior, not just execution.With the new
processTemplates/processFunctionsarguments, this test currently won’t catch forwarding/behavior regressions because results are ignored. Consider asserting a deterministic outcome (or removing this wrapper-only test if covered meaningfully elsewhere).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 `@pkg/list/list_instances_coverage_test.go` around lines 44 - 59, The wrapper test currently only executes processInstances and ignores outputs; update it to assert deterministic behavior for the new processTemplates/processFunctions flags: call processInstances with controlled inputs (e.g., a minimal AtmosConfiguration and explicit processTemplates/processFunctions boolean values), then assert the returned instances slice and/or error using errors.Is() or exact value comparisons, or replace the test with table-driven cases that cover the meaningful combinations of processTemplates/processFunctions and validate expected outputs; if this wrapper truly adds no behavior beyond processInstancesWithDeps, remove the tautological test and rely on the underlying processInstancesWithDeps tests instead.cmd/list/sources_test.go (1)
871-892: This options test is mostly tautological right now.It validates direct field assignment rather than command behavior. Prefer a regression test that proves these booleans influence the downstream describe/list path.
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 `@cmd/list/sources_test.go` around lines 871 - 892, TestSourcesOptions_ProcessTemplatesAndFunctions currently only checks that struct fields hold assigned values; change it to exercise the downstream describe/list path by invoking the command behavior that uses SourcesOptions. Create a small test double (mock/stub) for the component that performs listing/describing (e.g., a SourcesLister/Describer interface used by the command) and inject it into the code path invoked by the test, then call the real entrypoint (the function that consumes SourcesOptions and triggers the describe/list flow) and assert the mock observed whether templates and functions were processed according to ProcessTemplates and ProcessFunctions; keep the table-driven cases (both_on, templates_on_functions_off, etc.) and replace the tautological assert.Equal checks on the struct fields with assertions against the mock's recorded calls/flags to prove behavior rather than implementation.cmd/list/stacks_test.go (1)
444-465: Consider replacing this with a behavior-driven wiring test.This currently validates only direct struct assignment. A higher-signal test would assert these values affect downstream execution behavior.
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 `@cmd/list/stacks_test.go` around lines 444 - 465, The test TestStacksOptions_ProcessTemplatesAndFunctions currently only asserts direct struct field assignment on StacksOptions (ProcessTemplates, ProcessFunctions) which is tautological; replace it with a behavior-driven test that wires StacksOptions into the actual code path that consumes those flags (e.g., the command handler or function that executes stack processing), use dependency injection or test doubles to observe whether template and function processing ran (e.g., by injecting a mock processor and asserting its ProcessTemplates/ProcessFunctions call behavior), and assert observable side effects or interactions rather than field values; also ensure you use errors.Is() for any error comparisons in the new test.cmd/list/components_test.go (1)
1148-1169: Prefer behavior-level coverage over struct self-assignment checks.This test only reasserts values set in the same literal, so it won’t catch actual wiring regressions. Consider asserting end-to-end option propagation from flags/env into execution paths instead.
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 `@cmd/list/components_test.go` around lines 1148 - 1169, The test TestComponentsOptions_ProcessTemplatesAndFunctions only reasserts fields on the ComponentsOptions struct (ProcessTemplates/ProcessFunctions) and is tautological; replace it with a behavior-level test that exercises the code path which builds and consumes ComponentsOptions (e.g., invoke the CLI flag/env parsing or the component execution function that accepts ComponentsOptions) so you assert real option propagation: set flags or env to enable/disable templates/functions, call the function that constructs or uses ComponentsOptions, and then verify the resulting behavior (templates processed vs skipped, functions processed vs skipped) for each case instead of comparing struct fields directly.cmd/list/metadata_test.go (1)
41-62: Consider strengthening the options test beyond struct round-trip.
TestMetadataOptions_ProcessTemplatesAndFunctionsjust sets two bool fields and reads them back — that's testing Go's struct semantics, not the feature. The parallel test inpkg/list/list_instances_process_test.go(newTestProcessInstancesWithDeps_PropagatesTemplateAndFunctionFlags) is the right pattern: drive the options through the actual execution path and assert the flags reachExecuteDescribeStacks. Consider either dropping this table or replacing it with an end-to-end check thatMetadataOptions.ProcessTemplates/ProcessFunctionsactually flow into the downstream call.As per coding guidelines: "Test behavior, not implementation; never test stub functions; avoid tautological tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/metadata_test.go` around lines 41 - 62, The current TestMetadataOptions_ProcessTemplatesAndFunctions only verifies struct field assignment; update it to exercise the real execution path so flags propagate: create a test that constructs MetadataOptions with ProcessTemplates/ProcessFunctions set, invokes the same higher-level flow used in production (the command handler or runner that ultimately calls ExecuteDescribeStacks), and assert that ExecuteDescribeStacks receives the corresponding flags (use a test double/mocked ExecuteDescribeStacks to capture its input). Specifically, replace the tautological assertions on MetadataOptions with a call that routes options through the code path that calls ExecuteDescribeStacks and verify the flags are observed there (refer to MetadataOptions, ProcessTemplates, ProcessFunctions, and ExecuteDescribeStacks to locate and wire the test).cmd/list/instances_test.go (1)
328-353: Same tautology flag as inmetadata_test.go.
TestInstancesOptions_ProcessTemplatesAndFunctionsonly exercises struct-field assignment. The real value-add is the companion test inpkg/list/list_instances_process_test.go(TestProcessInstancesWithDeps_PropagatesTemplateAndFunctionFlags) which actually proves the flags flow intoExecuteDescribeStacks. Consider removing this table or replacing it with an end-to-end check thatInstancesOptions.ProcessTemplates/ProcessFunctionsreach the downstream call site.As per coding guidelines: "Test behavior, not implementation; never test stub functions; avoid tautological tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/instances_test.go` around lines 328 - 353, The test TestInstancesOptions_ProcessTemplatesAndFunctions is tautological (only checks struct field assignments); remove it or replace it with an end-to-end assertion that the flag values actually propagate to the downstream call site (e.g., ExecuteDescribeStacks). Concretely: delete TestInstancesOptions_ProcessTemplatesAndFunctions or rewrite it to invoke the command path (or the function that calls ExecuteDescribeStacks), set InstancesOptions.ProcessTemplates and ProcessFunctions, and assert the called ExecuteDescribeStacks (or the command RunE) receives/uses those flags—use a spy/mocked ExecuteDescribeStacks or inspect behavior in the same style as pkg/list/list_instances_process_test.go TestProcessInstancesWithDeps_PropagatesTemplateAndFunctionFlags to verify propagation rather than field assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-04-24-list-instances-per-component-auth.md`:
- Around line 145-150: Update the wording around the --process-functions flag to
reflect scope drift: find the paragraph that begins "Adding a
`--process-functions` flag to `list instances`" and change the text to state
that the flag was an "initial-fix non-goal" and that a later companion commit
added the flag, e.g., reword to "initial-fix non-goal; later companion commit
added `--process-functions` to `list instances`" so both the earlier non-goal
statement and the delivered change (documented later) are reconciled; ensure the
same clarified phrasing is applied where the flag is described again in the
later section that currently documents it as delivered.
In `@pkg/list/list_instances.go`:
- Around line 389-391: Update the stale comment that describes the default
behavior for the "list instances" implementation: change the wording to reflect
that both processTemplates and processYamlFunctions default to true, and note
that users can opt out of YAML function processing via the CLI flag
--process-functions=false; update any mention of "default shape" near the list
instances implementation (symbols: processTemplates, processYamlFunctions, the
list instances handler/function) to clearly state the current defaults and the
opt-out flag.
In `@website/docs/cli/commands/list/list-values.mdx`:
- Around line 52-56: The docs add `--process-templates` and
`--process-functions` to the list values page but the `list values` command
likely doesn't register those flags; either remove the two `<dt>/<dd>` blocks
from the docs or wire the flags into the values command by adding the same flag
registration calls used by the other list subcommands (e.g. add
WithProcessTemplatesFlag and WithProcessFunctionsFlag to the values
command/parser initialization, update cmd/list/values.go and add a unit test to
assert the flags are accepted), ensuring flag names
ProcessTemplates/ProcessFunctions appear in the values parser.
---
Outside diff comments:
In `@NOTICE`:
- Around line 1-1830: The NOTICE file was manually edited and is out-of-date;
regenerate it from the source-of-truth by running the provided generator and
commit the generated output: run ./scripts/generate-notice.sh, replace the
current NOTICE with the script's output, verify CI passes, and commit the
regenerated NOTICE (do not hand-edit the NOTICE file).
---
Nitpick comments:
In `@cmd/list/components_test.go`:
- Around line 1148-1169: The test
TestComponentsOptions_ProcessTemplatesAndFunctions only reasserts fields on the
ComponentsOptions struct (ProcessTemplates/ProcessFunctions) and is
tautological; replace it with a behavior-level test that exercises the code path
which builds and consumes ComponentsOptions (e.g., invoke the CLI flag/env
parsing or the component execution function that accepts ComponentsOptions) so
you assert real option propagation: set flags or env to enable/disable
templates/functions, call the function that constructs or uses
ComponentsOptions, and then verify the resulting behavior (templates processed
vs skipped, functions processed vs skipped) for each case instead of comparing
struct fields directly.
In `@cmd/list/instances_test.go`:
- Around line 328-353: The test
TestInstancesOptions_ProcessTemplatesAndFunctions is tautological (only checks
struct field assignments); remove it or replace it with an end-to-end assertion
that the flag values actually propagate to the downstream call site (e.g.,
ExecuteDescribeStacks). Concretely: delete
TestInstancesOptions_ProcessTemplatesAndFunctions or rewrite it to invoke the
command path (or the function that calls ExecuteDescribeStacks), set
InstancesOptions.ProcessTemplates and ProcessFunctions, and assert the called
ExecuteDescribeStacks (or the command RunE) receives/uses those flags—use a
spy/mocked ExecuteDescribeStacks or inspect behavior in the same style as
pkg/list/list_instances_process_test.go
TestProcessInstancesWithDeps_PropagatesTemplateAndFunctionFlags to verify
propagation rather than field assignment.
In `@cmd/list/metadata_test.go`:
- Around line 41-62: The current
TestMetadataOptions_ProcessTemplatesAndFunctions only verifies struct field
assignment; update it to exercise the real execution path so flags propagate:
create a test that constructs MetadataOptions with
ProcessTemplates/ProcessFunctions set, invokes the same higher-level flow used
in production (the command handler or runner that ultimately calls
ExecuteDescribeStacks), and assert that ExecuteDescribeStacks receives the
corresponding flags (use a test double/mocked ExecuteDescribeStacks to capture
its input). Specifically, replace the tautological assertions on MetadataOptions
with a call that routes options through the code path that calls
ExecuteDescribeStacks and verify the flags are observed there (refer to
MetadataOptions, ProcessTemplates, ProcessFunctions, and ExecuteDescribeStacks
to locate and wire the test).
In `@cmd/list/sources_test.go`:
- Around line 871-892: TestSourcesOptions_ProcessTemplatesAndFunctions currently
only checks that struct fields hold assigned values; change it to exercise the
downstream describe/list path by invoking the command behavior that uses
SourcesOptions. Create a small test double (mock/stub) for the component that
performs listing/describing (e.g., a SourcesLister/Describer interface used by
the command) and inject it into the code path invoked by the test, then call the
real entrypoint (the function that consumes SourcesOptions and triggers the
describe/list flow) and assert the mock observed whether templates and functions
were processed according to ProcessTemplates and ProcessFunctions; keep the
table-driven cases (both_on, templates_on_functions_off, etc.) and replace the
tautological assert.Equal checks on the struct fields with assertions against
the mock's recorded calls/flags to prove behavior rather than implementation.
In `@cmd/list/stacks_test.go`:
- Around line 444-465: The test TestStacksOptions_ProcessTemplatesAndFunctions
currently only asserts direct struct field assignment on StacksOptions
(ProcessTemplates, ProcessFunctions) which is tautological; replace it with a
behavior-driven test that wires StacksOptions into the actual code path that
consumes those flags (e.g., the command handler or function that executes stack
processing), use dependency injection or test doubles to observe whether
template and function processing ran (e.g., by injecting a mock processor and
asserting its ProcessTemplates/ProcessFunctions call behavior), and assert
observable side effects or interactions rather than field values; also ensure
you use errors.Is() for any error comparisons in the new test.
In `@pkg/list/list_instances_coverage_test.go`:
- Around line 44-59: The wrapper test currently only executes processInstances
and ignores outputs; update it to assert deterministic behavior for the new
processTemplates/processFunctions flags: call processInstances with controlled
inputs (e.g., a minimal AtmosConfiguration and explicit
processTemplates/processFunctions boolean values), then assert the returned
instances slice and/or error using errors.Is() or exact value comparisons, or
replace the test with table-driven cases that cover the meaningful combinations
of processTemplates/processFunctions and validate expected outputs; if this
wrapper truly adds no behavior beyond processInstancesWithDeps, remove the
tautological test and rely on the underlying processInstancesWithDeps tests
instead.
In `@pkg/list/list_metadata_test.go`:
- Around line 359-406: The tests
TestMetadataOptions_ProcessTemplatesAndFunctionsAllCombinations and
TestInstancesCommandOptions_ProcessTemplatesAndFunctions are tautological
because they only re-assert fields set in the test; change them to assert
behavior: construct a MetadataOptions or InstancesCommandOptions with each flag
combo, call the real processing function(s) that use those structs (e.g., the
metadata/instance processing pipeline functions that accept MetadataOptions or
InstancesCommandOptions), and assert side-effects for each flag (for example,
when ProcessTemplates is true templates are expanded/replaced and when false
they remain intact; when ProcessFunctions is true function calls are executed
and when false they are not). If needed, inject test doubles or hooks into the
processing functions to observe whether template or function handlers ran (use
DI or a small fake processor), and replace the current field-only assertions
with checks against the processed output or observed handler invocations for
each combo.
🪄 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: 1c7f95f4-6b0f-4e8e-81e0-5a2f2d2b598b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
NOTICEcmd/list/components.gocmd/list/components_test.gocmd/list/flag_wrappers.gocmd/list/flag_wrappers_test.gocmd/list/instances.gocmd/list/instances_test.gocmd/list/metadata.gocmd/list/metadata_test.gocmd/list/sources.gocmd/list/sources_test.gocmd/list/stacks.gocmd/list/stacks_test.godocs/fixes/2026-04-24-list-instances-per-component-auth.mdexamples/quick-start-advanced/Dockerfilego.modinternal/exec/describe_stacks_component_processor.gointernal/exec/describe_stacks_component_processor_auth_test.gopkg/ai/analyze/analyze_test.gopkg/devcontainer/lifecycle_rebuild_test.gopkg/list/list_instances.gopkg/list/list_instances_coverage_test.gopkg/list/list_instances_process_test.gopkg/list/list_metadata.gopkg/list/list_metadata_test.gowebsite/blog/2026-04-24-list-process-flags.mdxwebsite/docs/cli/commands/list/list-components.mdxwebsite/docs/cli/commands/list/list-instances.mdxwebsite/docs/cli/commands/list/list-metadata.mdxwebsite/docs/cli/commands/list/list-settings.mdxwebsite/docs/cli/commands/list/list-sources.mdxwebsite/docs/cli/commands/list/list-stacks.mdxwebsite/docs/cli/commands/list/list-values.mdxwebsite/docs/cli/commands/list/list-vars.mdxwebsite/src/data/roadmap.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2363 +/- ##
==========================================
+ Coverage 77.64% 77.96% +0.31%
==========================================
Files 1090 1090
Lines 102974 103072 +98
==========================================
+ Hits 79955 80358 +403
+ Misses 18660 18308 -352
- Partials 4359 4406 +47
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Addresses CodeRabbit review feedback and Codecov patch coverage for the list-flag rollout PR. CodeRabbit feedback: - Drop 7 tautological `*Options_ProcessTemplatesAndFunctions` tests that only validated struct field round-tripping (not behavior). Covered by the existing `TestProcessInstancesWithDeps_PropagatesTemplate…` mock test and the new parser-wiring tests. - Drop the explicit coverage-theater `TestProcessInstances` wrapper test. Its own comment labeled it "just executing to achieve coverage" — exactly the pattern the guidelines forbid. - Fix stale comment on `processInstancesWithDeps` that claimed the `(templates=true, functions=false)` combination was the "default shape" for list instances. Reworded to describe the actual CLI defaults (both `true`) and name `--process-functions=false` as the opt-out path. - Rewrite the fix doc: drop the Non-goals section (all three items were intermediary ideas that either shipped later or were never the shape of the final fix) and fold "Companion flag work (later commit)" into a plain "Flag surface" section describing what shipped, without the temporal framing. Codecov patch coverage: - Extract `parseInstancesOptions` / `parseComponentsOptions` / `parseMetadataOptions` / `parseSourcesOptions` / `parseStacksOptions` from their respective RunE closures in cmd/list/*.go. Each is a pure `(cmd, v [, args]) → *Options` function that can be unit-tested without driving cobra. - Add `cmd/list/parse_options_test.go` — 5 tests covering defaults and explicit flags for each parser, plus the positional-arg path for `parseSourcesOptions` and the tri-state `*bool` enabled/locked path for `parseComponentsOptions`. - Add `cmd/list/cmd_executor_integration_test.go` — 6 integration tests that exercise each cmd-layer executor (`executeListInstancesCmd`, `listComponentsWithOptions`, `executeListMetadataCmd`, `executeListSources`, `listStacksWithOptions`) against the existing `tests/fixtures/scenarios/complete` fixture. Covers the full ProcessCommandLineArgs → InitCliConfig → createAuthManagerForList → list.Execute* glue path including the new ProcessTemplates/ProcessFunctions pass-through lines. cmd/list package coverage: 56.6% (from a baseline where most executor functions were 0%). Executors now range from 21% to 76%, and all five parseOptions helpers are at 100%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cmd/list/cmd_executor_integration_test.go (2)
14-18: Nit: build the fixture path withfilepath.Join.The literal
"../../tests/fixtures/scenarios/complete"works becausego testruns from the package directory and Go stdlib accepts forward slashes on Windows for most ops, but the repo rules ask forfilepath.Joinover forward-slash concatenation for consistency.♻️ Suggested change
-const completeFixturePath = "../../tests/fixtures/scenarios/complete" +var completeFixturePath = filepath.Join("..", "..", "tests", "fixtures", "scenarios", "complete")As per coding guidelines: "Never use forward slash concatenation … always use
filepath.Join()with separate arguments."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/cmd_executor_integration_test.go` around lines 14 - 18, Replace the hard-coded forward-slash path in the constant completeFixturePath with a filepath.Join-based construction; update the file to import "path/filepath" and define completeFixturePath using filepath.Join("..", "..", "tests", "fixtures", "scenarios", "complete") (reference: completeFixturePath constant in cmd_executor_integration_test.go).
23-30: Preferrequireover manualt.Fatalfhere.Tiny style nit to match the rest of the new test code in this PR:
♻️ Suggested change
func initExecutorTestIO(t *testing.T) { t.Helper() ioCtx, err := iolib.NewContext() - if err != nil { - t.Fatalf("failed to initialize I/O context: %v", err) - } + require.NoError(t, err, "failed to initialize I/O context") ui.InitFormatter(ioCtx) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/cmd_executor_integration_test.go` around lines 23 - 30, The test helper initExecutorTestIO uses manual t.Fatalf for error handling; replace that with the require style used elsewhere: call iolib.NewContext(), then assert no error via require.NoError(t, err) (importing github.com/stretchr/testify/require) and remove the t.Fatalf branch, then continue to call ui.InitFormatter(ioCtx); this updates initExecutorTestIO, keeping iolib.NewContext and ui.InitFormatter usage but using require.NoError for cleaner, consistent test assertions.cmd/list/parse_options_test.go (1)
19-29: Use the parser's ownBindFlagsToViperinstead of re-implementing the loop.The hand-rolled loop here duplicates what production code (
stacksParser.BindFlagsToViper(cmd, v)incmd/list/stacks.go, same for sibling parsers) already does. IfBindFlagsToViperchanges semantics, these tests won't notice. Also aligns with the repo rule that commands should not touchviper.BindPFlagdirectly.♻️ Suggested change
-// bindFlagsToViper mirrors the production BindPFlag loop so the parse* -// helpers see viper values that match the cobra flag values. Returns a -// fresh viper.Viper each call to isolate tests from global viper state. -func bindFlagsToViper(t *testing.T, cmd *cobra.Command) *viper.Viper { - t.Helper() - v := viper.New() - cmd.Flags().VisitAll(func(f *pflag.Flag) { - require.NoError(t, v.BindPFlag(f.Name, cmd.Flags().Lookup(f.Name))) - }) - return v -} +// bindFlagsToViper returns a fresh viper.Viper bound to `cmd`'s flags via +// the same parser helper production uses, so the parse* helpers see the +// real binding semantics and tests are isolated from global viper state. +func bindFlagsToViper(t *testing.T, cmd *cobra.Command, parser *flags.StandardParser) *viper.Viper { + t.Helper() + v := viper.New() + require.NoError(t, parser.BindFlagsToViper(cmd, v)) + return v +}Call sites would then pass the matching parser (e.g.
bindFlagsToViper(t, cmd, componentsParser)), which also drops thepflagimport.As per coding guidelines: "Never use
viper.BindEnv()orviper.BindPFlag()directly; commands MUST useflags.NewStandardParser()for command-specific flags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/parse_options_test.go` around lines 19 - 29, The test helper re-implements flag binding; change bindFlagsToViper to accept the parser instance (e.g. componentsParser/stacksParser) and call that parser's BindFlagsToViper(cmd, v) instead of iterating flags and calling v.BindPFlag; keep creating a fresh v := viper.New() and return it, remove the pflag import and update all test call sites to pass the appropriate parser to bindFlagsToViper so tests use the parser's binding logic.cmd/list/stacks.go (1)
160-173: Consider: early-empty-stacks short-circuit also skips tree output.Minor behavioral note (pre-existing in this file, just highlighting since the tree path now depends on these inputs): when the initial
executeAndExtractStacksreturns zero stacks, we printNo stacks foundand return before the tree re-processing branch at Line 166–167. That's likely fine in practice because the same filters are applied again, but if a future change makes the tree path produce different selection semantics, this early return would mask it. Nothing to fix here — just flagging so the next person touching tree rendering knows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/stacks.go` around lines 160 - 173, The early-return when len(stacks) == 0 prevents the tree-specific path (opts.Format == string(format.FormatTree)) from executing, which can mask differences in tree rendering semantics; modify the logic in the function that calls executeAndExtractStacks so that the tree-format branch (check of opts.Format and call to renderStacksTreeFormat) is evaluated before short-circuiting on empty stacks (or handle the empty-stacks case inside renderStacksTreeFormat), ensuring renderStacksTreeFormat and its use of stacksMap still run for FormatTree even when stacks is empty; refer to opts.Format, format.FormatTree, renderStacksTreeFormat, and the len(stacks) check to locate and adjust the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/list/cmd_executor_integration_test.go`:
- Around line 51-75: The test TestExecuteListInstancesCmd_CoverageIntegration is
ignoring the executor result by doing `_ = err`, producing "coverage-theater";
update the test(s) that call executeListInstancesCmd (and similar
_CoverageIntegration tests) to either (A) treat the fixture as a well-formed
scenario: assert require.NoError(t, err) and capture/assert stdout (e.g., decode
the JSON output and assert a non-empty slice) while keeping InstancesOptions
(Format, ProcessTemplates, ProcessFunctions) as in the test, or (B) if the
fixture may legitimately error, mark the test with t.Skip(reason) or replace it
with a focused unit test using DI/mocks; apply the same change pattern to the
other listed tests to ensure they validate behavior instead of merely increasing
coverage.
In `@docs/fixes/2026-04-24-list-instances-per-component-auth.md`:
- Around line 329-330: Update the wording that states "hang in CI" to "fail in
CI" to match earlier descriptions of a deterministic credentials failure;
specifically edit the sentence containing "defaulting both to `true` safe;
without it, the `list instances` path would hang in CI as described in the Issue
section" so it instead reads that the `list instances` path would "fail in CI",
preserving the rest of the sentence and terminology consistency.
---
Nitpick comments:
In `@cmd/list/cmd_executor_integration_test.go`:
- Around line 14-18: Replace the hard-coded forward-slash path in the constant
completeFixturePath with a filepath.Join-based construction; update the file to
import "path/filepath" and define completeFixturePath using filepath.Join("..",
"..", "tests", "fixtures", "scenarios", "complete") (reference:
completeFixturePath constant in cmd_executor_integration_test.go).
- Around line 23-30: The test helper initExecutorTestIO uses manual t.Fatalf for
error handling; replace that with the require style used elsewhere: call
iolib.NewContext(), then assert no error via require.NoError(t, err) (importing
github.com/stretchr/testify/require) and remove the t.Fatalf branch, then
continue to call ui.InitFormatter(ioCtx); this updates initExecutorTestIO,
keeping iolib.NewContext and ui.InitFormatter usage but using require.NoError
for cleaner, consistent test assertions.
In `@cmd/list/parse_options_test.go`:
- Around line 19-29: The test helper re-implements flag binding; change
bindFlagsToViper to accept the parser instance (e.g.
componentsParser/stacksParser) and call that parser's BindFlagsToViper(cmd, v)
instead of iterating flags and calling v.BindPFlag; keep creating a fresh v :=
viper.New() and return it, remove the pflag import and update all test call
sites to pass the appropriate parser to bindFlagsToViper so tests use the
parser's binding logic.
In `@cmd/list/stacks.go`:
- Around line 160-173: The early-return when len(stacks) == 0 prevents the
tree-specific path (opts.Format == string(format.FormatTree)) from executing,
which can mask differences in tree rendering semantics; modify the logic in the
function that calls executeAndExtractStacks so that the tree-format branch
(check of opts.Format and call to renderStacksTreeFormat) is evaluated before
short-circuiting on empty stacks (or handle the empty-stacks case inside
renderStacksTreeFormat), ensuring renderStacksTreeFormat and its use of
stacksMap still run for FormatTree even when stacks is empty; refer to
opts.Format, format.FormatTree, renderStacksTreeFormat, and the len(stacks)
check to locate and adjust the code.
🪄 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: 2f4fa119-c831-4345-80c0-0dd1228cd3a6
📒 Files selected for processing (16)
cmd/list/cmd_executor_integration_test.gocmd/list/components.gocmd/list/components_test.gocmd/list/instances.gocmd/list/instances_test.gocmd/list/metadata.gocmd/list/metadata_test.gocmd/list/parse_options_test.gocmd/list/sources.gocmd/list/sources_test.gocmd/list/stacks.gocmd/list/stacks_test.godocs/fixes/2026-04-24-list-instances-per-component-auth.mdpkg/list/list_instances.gopkg/list/list_instances_coverage_test.gopkg/list/list_metadata_test.go
💤 Files with no reviewable changes (1)
- pkg/list/list_instances_coverage_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/list/list_instances.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/list/sources_test.go
- cmd/list/metadata_test.go
- pkg/list/list_metadata_test.go
- cmd/list/components_test.go
- cmd/list/instances.go
Addresses the latest round of CodeRabbit feedback plus Codecov patch coverage: CodeRabbit feedback: - Integration tests in cmd/list/cmd_executor_integration_test.go used `_ = err` which was coverage theater — they passed regardless of what the executor returned. Tightened all 5 to `require.NoError` with messages describing the expected behavior. Wiring had to change to match: register the global flags builder on the test cmd (so ProcessCommandLineArgs can read `base-path`/`config`/etc.) and init `data.InitWriter` in the test I/O setup. - `parse_options_test.go`: replaced the hand-rolled `v.BindPFlag` loop with `parser.BindFlagsToViper(cmd, v)` — the same helper production RunE closures use. Tests now exercise the real binding semantics and don't touch `viper.BindPFlag` directly (CLAUDE.md forbids that outside pkg/flags). - Fix doc + blog: reworded "hang in CI" / "hang on missing AWS credentials" to the actual failure mode users see (`No valid credential sources found`), consistent with the Issue section. Codecov patch coverage — new tests to hit previously-0% branches: - `cmd/list/cmd_executor_integration_test.go`: `TestListStacksWithOptions_TreeFormat`, `TestListStacksWithOptions_TreeFormatWithProvenance`, `TestExecuteListInstancesCmd_TreeFormat`, `TestExecuteListInstancesCmd_MatrixFormat`. - `pkg/list/list_metadata_test.go`: `TestExecuteListMetadataCmd` (happy path with the `complete` fixture) and `TestExecuteListMetadataCmd_InvalidConfig` (error path). Mirrors the existing `TestExecuteListInstancesCmd` pattern. - `pkg/list/list_instances_coverage_test.go`: `TestExecuteListInstancesCmd_TreeFormat` and `TestExecuteListInstancesCmd_TreeFormatRejectsUpload`. Coverage jumps (per function): renderStacksTreeFormat 0% → 86.7% resolveAndFilterImportTrees 0% → 84.6% pkg ExecuteListMetadataCmd 0% → 73.9% pkg ExecuteListInstancesCmd 51.6% → 68.8% cmd executeListInstancesCmd 35.7% → 78.6% cmd executeListMetadataCmd 23.1% → 76.9% cmd listStacksWithOptions 37.5% → 75.0% cmd/list package total 56.6% → 63.9% Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/list/list_metadata_test.go (2)
372-372: Usefilepath.Joinfor cross-platform fixture paths.Coding guidelines prohibit forward-slash-concatenated path literals in tests (
tempDir + "/components/...") and hardcoded Unix paths in favor offilepath.Join. Both the fixture path and the sentinel "invalid" path are better expressed portably.♻️ Proposed diff
- fixturePath := "../../tests/fixtures/scenarios/complete" + fixturePath := filepath.Join("..", "..", "tests", "fixtures", "scenarios", "complete") tests.RequireFilePath(t, fixturePath, "test fixture directory")- info := &schema.ConfigAndStacksInfo{ - BasePath: "/nonexistent/path", - } + info := &schema.ConfigAndStacksInfo{ + BasePath: filepath.Join(t.TempDir(), "does-not-exist"), + }(requires adding
"path/filepath"to the import block.)As per coding guidelines: "Never use forward slash concatenation ...; always use
filepath.Join()" and "Never hardcode Unix paths ... in tests".Also applies to: 397-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/list/list_metadata_test.go` at line 372, Replace hardcoded forward-slash fixture paths with filepath.Join for portability: import "path/filepath" in the test file and change the fixturePath variable (currently set to "../../tests/fixtures/scenarios/complete") to use filepath.Join(...) and likewise replace the sentinel "invalid" path literal referenced nearby (line with "invalid") to use filepath.Join segments; update any other test path literals in the same test (e.g., the one at the later mention) to use filepath.Join to satisfy the coding guideline.
400-403: Assert the specific error witherrors.Is.The current
require.Erroronly proves some error bubbled up — a future regression that swallows the config-init error and surfaces a render-time error instead would still pass. Pin it to the init-config sentinel (e.g.,errUtils.ErrInvalidConfigor whateverInitCliConfigreturns on invalid BasePath) so the test fails loudly on behavior drift.As per coding guidelines: "Test behavior, not implementation; ... use
errors.Is()for error checking".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/list/list_metadata_test.go` around lines 400 - 403, Replace the loose require.Error check with an assertion that the returned error matches the specific sentinel from InitCliConfig using errors.Is; locate the call to ExecuteListMetadataCmd and change the expectation to assert errors.Is(err, errUtils.ErrInvalidConfig) (or the exact sentinel value returned by InitCliConfig), importing the standard errors package and referencing errUtils (or the package that defines the sentinel) so the test fails only when the init-config sentinel is returned rather than any render-time error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/list/list_metadata_test.go`:
- Line 372: Replace hardcoded forward-slash fixture paths with filepath.Join for
portability: import "path/filepath" in the test file and change the fixturePath
variable (currently set to "../../tests/fixtures/scenarios/complete") to use
filepath.Join(...) and likewise replace the sentinel "invalid" path literal
referenced nearby (line with "invalid") to use filepath.Join segments; update
any other test path literals in the same test (e.g., the one at the later
mention) to use filepath.Join to satisfy the coding guideline.
- Around line 400-403: Replace the loose require.Error check with an assertion
that the returned error matches the specific sentinel from InitCliConfig using
errors.Is; locate the call to ExecuteListMetadataCmd and change the expectation
to assert errors.Is(err, errUtils.ErrInvalidConfig) (or the exact sentinel value
returned by InitCliConfig), importing the standard errors package and
referencing errUtils (or the package that defines the sentinel) so the test
fails only when the init-config sentinel is returned rather than any render-time
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11eaf63e-1620-4f82-9681-09d1098a0e6b
📒 Files selected for processing (7)
cmd/list/cmd_executor_integration_test.gocmd/list/instances_test.gocmd/list/parse_options_test.godocs/fixes/2026-04-24-list-instances-per-component-auth.mdpkg/list/list_instances_coverage_test.gopkg/list/list_metadata_test.gowebsite/blog/2026-04-24-list-process-flags.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/list/list_instances_coverage_test.go
- cmd/list/cmd_executor_integration_test.go
…s order-independent The test was failing on macOS CI while passing on Linux/Windows. Root cause: the `tests/fixtures/scenarios/invalid-stacks/` fixture contains several invalid YAML files with different error shapes (invalid template syntax in import paths, missing imports, malformed schemas, …). Filesystem walk order differs across OSes — on Linux/Windows the test happened to hit a file whose error contained "invalid", on macOS it hit `invalid-template-import-path.yaml` first and got `function "unclosed" not defined`, no "invalid" substring. The fixture file `invalid-template-import-path.yaml` was added in PR #2173 (now in main); my branch's test was relying on file walk order that no longer holds. Replace the brittle `assert.ErrorContains(t, err, "invalid")` with a multi-pattern check that accepts any of the documented error messages the fixture's invalid files can surface. Test still asserts an error is returned (`require.Error`); the substring check now tolerates the full set of error shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/exec/terraform_test.go (1)
360-377: Nice de-flake — sensible multi-pattern fallback.The
require.Errorplus lowercased OR-of-substrings approach is a clean way to absorb OS-dependent walk order without dropping the assertion entirely. The inline comment explaining why the brittleness existed is also a helpful breadcrumb for the next person who touches this.One small thought you can take or leave:
"function"and"template"are pretty broad fragments and would match a lot of unrelated future errors, so this assertion is closer to "we got some error" than "we got an invalid-stacks error." If you want a touch more signal without re-introducing brittleness, narrowing to the more specific phrases (e.g."invalid template","unclosed","no matches found","function \"") would still tolerate the documented variants while catching accidental error-shape drift. Totally optional givenrequire.Erroralready guards the main contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_test.go` around lines 360 - 377, The current test accepts very broad substrings ("function" and "template") in the errMsg check, which weakens the assertion; update the assertion in terraform_test.go (the require.Error/err, errMsg := strings.ToLower(err.Error()) block and the assert.True call) to look for narrower phrases that still cover documented variants — e.g. check for "invalid template" instead of "template" and for 'function "' (function followed by a quote) instead of bare "function", while keeping the existing "unclosed" and "no matches found" alternatives so the assertion remains tolerant of OS-dependent walk order.
🤖 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_test.go`:
- Around line 360-377: The current test accepts very broad substrings
("function" and "template") in the errMsg check, which weakens the assertion;
update the assertion in terraform_test.go (the require.Error/err, errMsg :=
strings.ToLower(err.Error()) block and the assert.True call) to look for
narrower phrases that still cover documented variants — e.g. check for "invalid
template" instead of "template" and for 'function "' (function followed by a
quote) instead of bare "function", while keeping the existing "unclosed" and "no
matches found" alternatives so the assertion remains tolerant of OS-dependent
walk order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e7c60fa-c50e-4da6-a939-7e177d48feea
📒 Files selected for processing (1)
internal/exec/terraform_test.go
Addresses two CodeRabbit comments on the new pkg-level metadata executor tests: 1. Replace forward-slash literal fixture path `"../../tests/fixtures/scenarios/complete"` with `filepath.Join(...)`, and replace the hardcoded Unix path `"/nonexistent/path"` with `filepath.Join(t.TempDir(), "does-not-exist")` for cross-platform portability and to avoid collisions with any real path. Per CLAUDE.md: "Never use forward slash concatenation; always use filepath.Join()" and "Never hardcode Unix paths in tests". 2. Tighten `TestExecuteListMetadataCmd_InvalidConfig` to assert `errors.Is(err, errUtils.ErrFailedToInitConfig)` instead of `require.Error` only, so a future regression that swallows the init-config error and surfaces a render-time error instead would fail loudly. Per CLAUDE.md: "use errors.Is() for error checking". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
requested a review
from Erik Osterman (Cloud Posse) (osterman)
April 25, 2026 00:51
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Apr 27, 2026
Contributor
Contributor
|
Warning Release Documentation RequiredThis PR is labeled
|
|
These changes were released in v1.217.0-rc.2. |
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
--process-templatesand--process-functionsCLI flags (andATMOS_PROCESS_TEMPLATES/ATMOS_PROCESS_FUNCTIONSenv vars) to everyatmos listsubcommand that processes stack manifests:list instances,list components,list metadata,list sources,list stacks. Defaults aretrue, matchingatmos describe affected/atmos describe stacks/atmos describe component.--process-templatestoggles Go templates (includingatmos.Component(...));--process-functionstoggles YAML functions (!terraform.state,!terraform.output,!store,!aws.*, …).atmos list instances --uploadhang in CI: per-component auth resolution ininternal/exec/describe_stacks_component_processor.gowas gated onprocessYamlFunctionsonly, so the template-only path (atmos.Component(...)inside Go templates) ranterraform initwith an emptyAuthContextagainst remote backends and failed withNo valid credential sources found. Guard now fires when either templates or YAML functions will run.shouldResolvePerComponentAuth(...)predicate,resolveComponentAuthManager(...)method, and an injectablecomponentAuthManagerResolverfield ondescribeStacksProcessorso the decision can be exercised without running real OIDC/STS.InstancesCommandOptions/MetadataOptionsinpkg/list/and through both the matrix-format and tree-format branches oflist_instances.go, so every output path of the same invocation honors the same flag values.ExecuteDescribeStacks) plus a dedicated auth-guard regression suite (TestShouldResolvePerComponentAuth,TestResolveComponentAuthManager6-row table,TestResolveComponentAuthManager_ResolverErrorFallsBackToParent).atmos listcommand page, added a blog post announcing the feature, and added a shipped milestone to the Discoverability & List Commands roadmap initiative.go.mod:sentry-go v0.45.1(cockroachdb/errors v1.12.0 still references the removedExtrafield),gocloud.dev v0.41.0(gomplate/v3 s3blob uses removedConfigProvider),hairyhenderson/go-fsimpl v0.3.1(transitive via the gocloud.dev pin).why
atmos list instances --uploadwas broken in CI for any repo whose component sections callatmos.Component(...)inside Go templates with a stack-level default identity — the exact shape used by the Atmos Pro release workflow. Users reported the command failing withNo valid credential sources foundwhileatmos describe affected --uploadin the same workflow succeeded.atmos.Component(...)is a Go template function, not a YAML function. The processor's per-component auth resolver assumed YAML functions were the only consumer ofinfo.AuthContextand gated itself onprocessYamlFunctions. The template path reads the sameAuthContextand shells out toterraform init+terraform output, so disabling per-component auth broke template-only invocations.atmos listflags to line up withatmos describeflags. They didn't: onlylist affected,list settings, andlist valueshad the two knobs. A user workflow actually relied on--process-functionsonlist instances(where it didn't exist), which produced anunknown flagerror and a confusing escape hatch. Adding the two flags everywhere the command processes stacks closes that gap.truefor parity. Users who runatmos listlocally withouttofu/terraformon$PATHcan opt out with--process-functions=falseorATMOS_PROCESS_FUNCTIONS=false; the auth-guard fix above ensures thetrue, truedefault works end-to-end in CI.go get -u ./...pass doesn't trip over them blindly.references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores