Fix YQ defaults for terraform.state/output YAML functions#1836
Conversation
- Add sentinel errors (ErrTerraformStateNotProvisioned, ErrTerraformOutputNotFound) to classify recoverable errors - Refactor terraform.state and terraform.output functions to return (any, error) instead of calling os.Exit() - Implement YQ default evaluation when components are not provisioned or outputs don't exist - Propagate errors up call stack instead of deep exits via CheckErrorPrintAndExit - Add comprehensive tests for YQ defaults with terraform.state and terraform.output - Verify YQ correctly handles null values and list/map defaults Fixes sporadic failures with YQ expression defaults in YAML functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces error-aware YAML function processing for terraform.state and terraform.output with recoverable error classification and YQ default value fallbacks. It migrates logging from charmbracelet to an internal logger package, adds structured logging field constants, enhances backend retry logging, and updates related tests and documentation to reflect these changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
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 |
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Log error codes (S3 API error codes, GCS gRPC status codes, Azure HTTP status codes) when all retries fail to help users report backend issues. Also fixes GCS backend to use the correct atmos logger package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
bot
added
size/xl
|
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. |
Change from fixed 2-second backoff to exponential backoff (1s, 2s, 4s) for consistency with Azure backend. This reduces load on backends during transient failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace direct imports of github.com/charmbracelet/log with the atmos logger wrapper (github.com/cloudposse/atmos/pkg/logger) in: - pkg/profiler/profiler.go - pkg/hooks/store_cmd.go - internal/exec/spinner_utils.go Fix linter warnings: - Use %w instead of %v for error wrapping in yaml_func_terraform_state.go - Add logFieldFunction constant for repeated "function" string literal - Add logFieldBucketGCS constant for repeated "bucket" string literal in GCS - Add logFieldBucketS3 constant for repeated "bucket" string literal in S3 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- pkg/auth/hooks.go: Use log.Level instead of charm.Level - pkg/auth/hooks_test.go: Remove charm import, use log.Level and log.*Level constants throughout test cases - pkg/store/artifactory_store_test.go: Update comments to reference atmos logger, use log.TraceLevel instead of hardcoded -4 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Introduce pkg/logger/fields.go with common log field keys for consistent structured logging. This enables: - Consistency: Same field name everywhere (no bucket vs s3_bucket) - Discoverability: Autocomplete shows available fields - Refactoring: Change a field name in one place Update modified files to use the new constants: - internal/terraform_backend/terraform_backend_s3.go: Use log.FieldBucket - internal/terraform_backend/terraform_backend_gcs.go: Use log.FieldBucket - internal/exec/yaml_func_terraform_output.go: Use log.FieldFunction 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The nested AuthManager propagation tests were failing because they tried to read actual terraform state files which don't exist in the test fixture. This commit: - Adds setupMockStateGetter helper to mock terraform state lookups - Updates all tests with ProcessYamlFunctions: true to use the mock - Fixes fixture atmos.yaml to use correct component path - Simplifies test.yaml by removing auth sections (tests use mocked AuthManager) The tests now pass without requiring actual terraform state files while still verifying AuthManager propagation through nested YAML function calls. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The describe_component_authmanager_propagation_test.go tests verify that component-level auth sections exist with specific identities. The previous commit inadvertently removed these auth sections when simplifying the fixture for the nested auth manager tests. This restores: - auth-override-level2 with test-level2-identity - multi-auth-level2 with account-b-identity - multi-auth-level1 with account-c-identity - mixed-override-component with mixed-override-identity - deep-level3 with deep-level3-identity - deep-level1 with deep-level1-identity Added a note in the fixture header explaining that tests with ProcessYamlFunctions: true must mock the stateGetter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/terraform_backend/terraform_backend_s3.go (1)
134-164: Deferred context cancel inside loop causes resource leak.Each loop iteration creates a new context but
defer cancel()won't execute until the function returns. This leaks contexts during retries.for attempt := 0; attempt <= maxRetryCount; attempt++ { // 30 sec timeout to read the state file from the S3 bucket. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() output, err := s3Client.GetObject(ctx, &s3.GetObjectInput{ Bucket: aws.String(bucket), Key: aws.String(tfStateFilePath), }) if err != nil { + cancel() // Check if the error is because the object doesn't exist. // If the state file does not exist (the component in the stack has not been provisioned yet), return a `nil` result and no error. var nsk *types.NoSuchKey if errors.As(err, &nsk) { log.Debug("Terraform state file doesn't exist in the S3 bucket; returning 'null'", "file", tfStateFilePath, log.FieldBucket, bucket) return nil, nil } lastErr = err if attempt < maxRetryCount { // Exponential backoff: 1s, 2s, 4s for attempts 0, 1, 2. backoff := time.Second * time.Duration(1<<attempt) log.Debug("Failed to read Terraform state file from the S3 bucket", "attempt", attempt+1, "file", tfStateFilePath, log.FieldBucket, bucket, "error", err, "backoff", backoff, ) time.Sleep(backoff) continue } // Retries exhausted - log warning with error details to help diagnose the issue. logS3RetryExhausted(err, tfStateFilePath, bucket, maxRetryCount) return nil, fmt.Errorf("%w: %v", errUtils.ErrGetObjectFromS3, lastErr) } + cancel() content, err := io.ReadAll(output.Body)
🧹 Nitpick comments (4)
internal/exec/yaml_func_resolution_context_bench_test.go (1)
117-131: Benchmark adaptation to newprocessNodessignature looks fine.Discarding the error in this benchmark is reasonable since the goal here is to measure overhead, not validate correctness, which should be covered by regular tests. If you ever want extra safety, you could add a non-benchmark test that asserts
processNodesdoesn’t error for this input.internal/terraform_backend/terraform_backend_azurerm.go (1)
222-304: Azure retry‑exhaustion logging is solid; consider reusing the shared wrap format.The new
logAzureRetryExhaustedhelper nicely surfaces status code, timeout vs non-timeout, file, container, and attempt count, without changing the retry behavior. That will make diagnosing backend issues much easier.If you want to tighten consistency further, you could consider using
errUtils.ErrWrapFormatinstead of this file’s localerrWrapFormatwhen wrappingErrGetBlobFromAzure, so all backend callers share the same wrapping convention. Not required for this PR.internal/exec/yaml_func_utils.go (1)
63-109: Error short-circuiting pattern works but has a subtlety.The
firstErrcheck at the start ofrecurseprevents new processing, but the loop at lines 100-102 continues iterating and callingrecurseeven after an error is captured. While the result is discarded (lines 104-106), you're doing unnecessary work.Consider breaking early:
for k, v := range data { newMap[k] = recurse(v) + if firstErr != nil { + break + } }This is a minor optimization—current code is correct.
pkg/logger/fields.go (1)
1-28: Good addition for logging consistency.Centralizing log field keys prevents typos and enables refactoring. The grouping (Infrastructure, Atmos domain, Operation, General) is logical. The constants are already adopted in some files (e.g.,
internal/exec/yaml_func_terraform_output.gousesFieldFunction), though broader migration from raw string literals remains incomplete—this is expected follow-up work as the codebase gradually adopts these constants.
1. describe_component_nested_authmanager_test.go:
- Fix comment periods to satisfy godot linter
- Improve comment clarity for scenario descriptions
2. yaml_func_terraform_output.go:
- Only use YQ defaults for recoverable terraform errors (state not
provisioned, output not found) - consistent with yaml_func_terraform_state.go
- Non-recoverable errors (API failures, auth errors) now fail hard
instead of silently using defaults
3. yq_utils_test.go:
- Replace "BUG: Currently..." wording with "Regression guard:..."
- Ensures tests document guarded behavior, not active bugs
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Updated TestTerraformOutput_APIErrorWithDefaultUsesDefault to properly test the behavior with recoverable vs non-recoverable errors: 1. Renamed to TestTerraformOutput_RecoverableErrorWithDefaultUsesDefault - Uses ErrTerraformStateNotProvisioned (recoverable error) - Expects YQ default to be used 2. Added TestTerraformOutput_APIErrorWithDefaultReturnsError - Uses ErrGetObjectFromS3 (non-recoverable API error) - Expects error to propagate even with YQ default present This aligns with the error handling semantics defined in errors.go: - Recoverable errors (state not provisioned, output not found) → use YQ default - API/infrastructure errors → fail hard, don't mask with defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflict in internal/exec/spinner_utils.go by keeping both imports: - log "github.com/cloudposse/atmos/pkg/logger" (from branch) - "github.com/cloudposse/atmos/pkg/terminal" (from main) Both are needed: log for Debug/Error calls, terminal for EscCarriageReturn. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/spinner_utils.go (1)
55-61: Avoid directfmtoutput to stdout/stderr; useui/datahelpersThe fallback paths use
fmt.Fprintln(os.Stderr, message)(Line 60) andfmt.Println(message)(Line 77). The coding guidelines explicitly discourage direct writes to stdout/stderr and require routing user-visible output throughdata.*orui.*helpers instead. This keeps output formatting and redirection consistent across the CLI.Consider switching these to the appropriate
ui.*(for user-facing messages) ordata.*(for pipeable data) calls while keeping the same behavior.
As per coding guidelines, avoid directfmtwrites to stdout/stderr.Also applies to: 75-79
♻️ Duplicate comments (2)
internal/exec/describe_component_nested_authmanager_test.go (1)
14-100: Centralized Terraform state mocking helper looks good; fix godot comment punctuation and consider minor reuse tweaks.The
setupMockStateGetterhelper is a nice way to centralizestateGettermocking and safely restore the global via the returned cleanup, and the expectations cover the nested auth fixtures well.A couple of nits:
- The leading multi-line comment on Lines 14–17 still has lines that do not end with a period, which will trip the
godotlinter. You can tighten it up like this:-// setupMockStateGetter configures the global stateGetter to return mock values -// for terraform state lookups. This allows tests to run without actual terraform -// state files. Returns a cleanup function that must be deferred to restore the -// original state getter. +// setupMockStateGetter configures the global stateGetter to return mock values for terraform state lookups. +// This allows tests to run without actual terraform state files. +// Returns a cleanup function that must be deferred to restore the original state getter.
- Optional: if you ever need this helper for a different stack than
"test", consider parameterizing the stack name so the expectations remain reusable.As per coding guidelines, all comment lines should end with periods.
pkg/utils/yq_utils_test.go (1)
174-258: Great YQ default coverage; tighten comments for godot and consider table-driven consolidation.These tests nicely pin down YQ
//behavior for nil data, empty maps, list/map defaults, nested keys, existing keys, and null values, which directly supports the new Terraform YAML-function semantics.Two small follow‑ups:
- A few comment lines still lack trailing periods (for example the first lines of several blocks like
TestEvaluateYqExpression_NilDataWithDefault verifies that YQ default,Regression guard: Ensures nil input with list defaults works correctly, etc.). The repo’sgodotrule wants each comment line to end with.to keep lint green.- Optional: if you keep adding cases, you might want to collapse these into a single table‑driven test over
{name, data, expr, want}to reduce duplication and make it easier to extend.
🧹 Nitpick comments (4)
internal/exec/describe_component_nested_authmanager_test.go (1)
392-423: setupNestedAuthTest nicely centralizes nested-auth setup; consider using t.Cleanup for teardown.Extending
setupNestedAuthTestto install the mock state getter and return astateCleanupclosure, alongside the controller andAuthManager, is a clean way to DRY up the scenario tests and ensure consistent mocking of Terraform state.If you want to further reduce boilerplate and the risk of forgetting cleanup at call sites, you could register both
ctrl.Finish()andstateCleanup()viat.Cleanup(...)insidesetupNestedAuthTest(and/orsetupMockStateGetter) instead of returning the cleanup function, so tests only need thetypes.AuthManager(and maybectrlif they still want it). Current approach is perfectly fine; this would just tighten the lifecycle to be more self-contained.internal/exec/spinner_utils.go (2)
47-50: Minor godot nit: add trailing period to comment lineLine 49’s comment doesn’t end with a period (
stderrhas no.), which will trip thegodotlinter. Suggest adding a trailing period to keep comments consistent with the enforced style.
As per coding guidelines, all comments must end with periods.
47-87: Optional: addperf.Trackto exported spinner helpersThe exported helpers
NewSpinner,RunSpinner, andStopSpinnerdon’t currently includedefer perf.Track(... )()instrumentation. If you’re touching this area anyway, it might be a good place to add perf tracking for consistency with the guideline that public functions should be instrumented.Not blocking for this PR, but worth considering as a follow-up or when this file is next updated.
As per coding guidelines, public functions should useperf.Track.internal/exec/yaml_func_terraform_output.go (1)
55-108: Error propagation and YQ-default handling align with the new design; consider logging YQ failures for recoverable errors.Nice improvements here:
processTagTerraformOutputWithContextnow returns(any, error), cleanly propagating failures fromgetStringAfterTag,SplitStringByDelimiter, and argument validation (wrapped withErrYamlFuncInvalidArguments).- Dependency tracking via
trackOutputDependency+ deferred cleanup is clear.- The
GetOutputerror handling now correctly gates defaults behindisRecoverableTerraformError(err) && hasYqDefault(output), so API/infrastructure failures no longer get silently “fixed” by defaults.- The
!existsbranch keeps backward compatibility while still honoring YQ defaults when present.One small tweak: in the recoverable‑error branch, if
evaluateYqDefaultfails you immediately return the original Terraform error without any indication that the YQ default evaluation also failed, while the!existsbranch logsyqErrat debug level. For easier troubleshooting, you could mirror that logging here:- defaultValue, yqErr := evaluateYqDefault(atmosConfig, output) - if yqErr != nil { - // If YQ evaluation fails, return the original error. - return nil, fmt.Errorf("failed to get terraform output for component %s in stack %s, output %s: %w", component, stack, output, err) - } + defaultValue, yqErr := evaluateYqDefault(atmosConfig, output) + if yqErr != nil { + log.Debug("YQ default evaluation failed for recoverable error; returning original terraform error", + log.FieldFunction, input, + "component", component, + "stack", stack, + "output", output, + "error", yqErr.Error(), + ) + // If YQ evaluation fails, return the original error. + return nil, fmt.Errorf("failed to get terraform output for component %s in stack %s, output %s: %w", component, stack, output, err) + }That keeps behavior identical while exposing why a configured default didn’t take effect.
Also applies to: 118-165
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
errors/errors.go(1 hunks)internal/exec/describe_component_nested_authmanager_test.go(9 hunks)internal/exec/spinner_utils.go(1 hunks)internal/exec/yaml_func_terraform_output.go(6 hunks)internal/exec/yaml_func_terraform_output_yq_defaults_test.go(1 hunks)pkg/utils/yq_utils_test.go(2 hunks)tests/fixtures/scenarios/authmanager-nested-propagation/stacks/test.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/scenarios/authmanager-nested-propagation/stacks/test.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- errors/errors.go
- internal/exec/yaml_func_terraform_output_yq_defaults_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
internal/exec/spinner_utils.gointernal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.gointernal/exec/yaml_func_terraform_output.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Useassert.ErrorIs(t, err, ErrSentinel)for checking our own errors and stdlib errors. Only use string matching for third-party errors
NEVER useassert.Contains(err.Error(), ...)for error checking. ALWAYS useassert.ErrorIs()instead. NEVER use string comparison:err.Error() == "..."orstrings.Contains(err.Error(), ...)
Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoidos.Exit,CheckErrorPrintAndExit, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Usego.uber.org/mock/mockgenwith//go:generatedirectives for mock generation. Never create manual mocks. Pattern://go:generate go run go.uber.org/mock/mockgen@latest -source=filename.go -destination=mock_filename_test.go
Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Files:
internal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Business logic packages must follow the Service-Oriented Architecture pattern with: one Service struct per domain, Provider interfaces for classes of problems, default implementations, mock implementations, and dependency injection for testability
Use interface-driven design: define interfaces for all major functionality, use dependency injection for testability, generate mocks withgo.uber.org/mock/mockgenwith//go:generatedirectives, avoid integration tests by mocking external dependencies
Create focused purpose-built packages inpkg/for new functionality, each with clear responsibility. DO NOT add new functions topkg/utils/orinternal/exec/. Each package should have focused business logic with tests
Implement registry pattern for extensibility and plugin-like architecture. Define interfaces for provider implementations, register implementations in registry, generate mocks with mockgen. Examples: Command Registry incmd/internal/registry.go, Store Registry inpkg/store/registry.go
Files:
pkg/utils/yq_utils_test.go
🧠 Learnings (47)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-02-21T20:56:20.761Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-01-28T21:30:30.063Z
Learnt from: mcalhoun
Repo: cloudposse/atmos PR: 980
File: internal/exec/template_funcs_gomplate_datasource.go:7-8
Timestamp: 2025-01-28T21:30:30.063Z
Learning: The logging wrappers in the codebase will be removed in a future PR to directly use charmbracelet logger, as part of the ongoing migration to standardize logging format.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/spinner_utils.gointernal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-01-30T16:56:43.004Z
Learnt from: mcalhoun
Repo: cloudposse/atmos PR: 980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
internal/exec/spinner_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to tests/**/*.go : Use `cmd.NewTestKit(t)` for any test touching RootCmd. This auto-cleans RootCmd state. Tests skip gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
internal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
internal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to pkg/**/*.go : Use interface-driven design: define interfaces for all major functionality, use dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen` with `//go:generate` directives, avoid integration tests by mocking external dependencies
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to tests/**/*.yaml : NEVER modify `tests/test-cases/` or `tests/testdata/` unless explicitly instructed. Golden snapshots are sensitive to minor changes. Use fixtures in `tests/test-cases/`: `atmos.yaml`, `stacks/`, `components/`
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
internal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.go
📚 Learning: 2025-03-25T12:23:42.649Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: internal/exec/go_getter_utils.go:104-104
Timestamp: 2025-03-25T12:23:42.649Z
Learning: Listener430 plans to add a test for verifying that token injection is skipped for unsupported hosts in a future review or refactoring iteration. This relates to the CustomGitDetector.Detect method in internal/exec/go_getter_utils.go.
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Applied to files:
internal/exec/describe_component_nested_authmanager_test.gopkg/utils/yq_utils_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
internal/exec/describe_component_nested_authmanager_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2025-01-19T23:13:50.429Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 943
File: internal/exec/describe_config.go:37-37
Timestamp: 2025-01-19T23:13:50.429Z
Learning: When reviewing pointer usage with `EvaluateYqExpression`, check if the `atmosConfig` parameter is already a pointer (e.g., when returned by `InitCliConfig`) to avoid suggesting unnecessary address-of operations.
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2025-09-30T19:03:50.738Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Applied to files:
pkg/utils/yq_utils_test.gointernal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Ensure all tests pass, verify code coverage meets targets, run golangci-lint and fix any issues, and update documentation before submitting pull requests
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : NEVER use `assert.Contains(err.Error(), ...)` for error checking. ALWAYS use `assert.ErrorIs()` instead. NEVER use string comparison: `err.Error() == "..."` or `strings.Contains(err.Error(), ...)`
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use `assert.ErrorIs(t, err, ErrSentinel)` for checking our own errors and stdlib errors. Only use string matching for third-party errors
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Use dependency injection and interface-driven design to make code testable - avoid `os.Exit`, `CheckErrorPrintAndExit`, and direct external system calls in testable functions
Applied to files:
pkg/utils/yq_utils_test.go
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-11-30T22:07:08.610Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-12-03T05:18:49.169Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2024-12-03T05:18:49.169Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-11-24T19:13:10.287Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-04-24T01:40:13.576Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:40:13.576Z
Learning: When processing YAML nodes with custom directives in Go using gopkg.in/yaml.v3, setting node.Tag = "" is sufficient to prevent re-processing of the node. It's not necessary to also clear node.Value after updating the configuration store (e.g., Viper), as the original value doesn't affect subsequent operations once the tag is removed.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
internal/exec/yaml_func_terraform_output.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
internal/exec/yaml_func_terraform_output.go
🧬 Code graph analysis (2)
internal/exec/describe_component_nested_authmanager_test.go (1)
internal/exec/mock_terraform_state_getter.go (1)
NewMockTerraformStateGetter(32-36)
internal/exec/yaml_func_terraform_output.go (7)
pkg/schema/schema.go (1)
AtmosConfiguration(53-96)internal/exec/yaml_func_resolution_context.go (1)
ResolutionContext(23-26)pkg/perf/perf.go (1)
Track(121-138)pkg/logger/log.go (1)
Debug(24-26)pkg/logger/fields.go (1)
FieldFunction(15-15)pkg/utils/yaml_utils.go (1)
AtmosYamlFuncTerraformOutput(26-26)errors/errors.go (1)
ErrYamlFuncInvalidArguments(92-92)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/describe_component_nested_authmanager_test.go (2)
125-127: Good use of setupMockStateGetter in the level 1 and level 2 propagation tests.Wiring
cleanup := setupMockStateGetter(t, ctrl)and deferring the cleanup in bothTestNestedAuthManagerPropagationandTestNestedAuthManagerPropagationLevel2correctly isolates the tests from real Terraform state and ensuresstateGetteris restored after each test run. The pattern with per-testgomock.Controllerplus cleanup looks sound.Also applies to: 189-191
442-445: Scenario 2–5 tests correctly reuse the shared nested-auth setup and state cleanup.The switch to
setupNestedAuthTestin the scenario tests, along with deferring bothctrl.Finish()andstateCleanup(), keeps the nested auth scenarios consistent with the new mocking strategy and avoids hitting real Terraform backends/IMDS. The pattern is clear and repeatable across all four tests.Also applies to: 481-484, 519-522, 560-563
internal/exec/spinner_utils.go (1)
10-13: Logger import swap looks goodUsing the internal
pkg/loggerwith aliasloghere is consistent with the project’s logging migration and existinglog.Debug/log.Errorcall patterns; no further changes needed in this file.
Based on learnings, thelogalias matches the repo convention.pkg/utils/yq_utils_test.go (1)
7-7: Require import usage looks good.Bringing in
requirealongsideassertis a nice fit for these tests that must bail out on error setup before checking results.internal/exec/yaml_func_terraform_output.go (1)
26-53: trackOutputDependency refactor and nil-context handling look solid.Returning
(func(), error)with a no‑op cleanup whenresolutionCtxis nil and pairingPush/Popvia the deferred cleanup keeps cycle detection explicit and avoids nil derefs, while letting simple call sites skip resolution context entirely.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Resolved conflict in internal/exec/yaml_func_utils.go by merging: - Error tuple signature (any, bool, error) from PR branch processSimpleTags - AWS YAML function handlers from main branch, updated with nil error returns Updated internal/exec/yaml_func_aws_test.go to use the new 3-value return signature for processSimpleTags function calls. The AWS YAML functions (!aws.account_id, !aws.caller_identity_arn, !aws.caller_identity_user_id, !aws.region) are now integrated into the processSimpleTags function with the correct error tuple return signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflict in internal/exec/yaml_func_utils.go: - Added new AWS YAML functions from main (!aws.account_id, !aws.caller_identity_arn, !aws.caller_identity_user_id, !aws.region) - Adapted to use 3-value return signature (result, handled, error) used by our branch Updated internal/exec/yaml_func_aws_test.go: - Fixed test calls to processSimpleTags to use 3 return values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…into osterman/yq-defaults-bug-tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
errors/errors.go (1)
97-115: Nice split between recoverable YAML errors and fatal backend/API errorsThis block cleanly encodes the contract you need for YQ
//defaults:ErrTerraformStateNotProvisioned/ErrTerraformOutputNotFoundas recoverable, andErrTerraformBackendAPIErrorplus the other backend sentinels as non-recoverable. Naming, wording, and comments all match the existing style and the error‑sentinel guidelines for this repo. If you want to be extra explicit later, you could consider renaming the second comment to “Non‑recoverable backend errors” to reflect that it also covers config issues, but that’s purely cosmetic and not required for this PR.internal/exec/yaml_func_aws_test.go (1)
447-478: Test updates look good, but consider adding error path coverage.The mechanical updates to capture and assert on errors are correct. However, these tests only verify successful AWS identity lookups. Consider adding test cases where
getAWSCallerIdentityCachedfails (e.g., network errors, invalid credentials) to verify error propagation works end-to-end with the refactored error handling.Based on coding guidelines, test coverage should target >80% and test behavior including error paths.
Add test cases like:
func TestProcessSimpleTagsAWSFunctionsWithErrors(t *testing.T) { ClearAWSIdentityCache() expectedErr := errors.New("AWS API error") restore := SetAWSGetter(&mockAWSGetter{ identity: nil, err: expectedErr, }) defer restore() atmosConfig := &schema.AtmosConfiguration{} stackInfo := &schema.ConfigAndStacksInfo{} result, handled, err := processSimpleTags(atmosConfig, u.AtmosYamlFuncAwsAccountID, "", nil, stackInfo) // Verify error is propagated based on actual implementation // If AWS functions should return errors, assert error here // If they should return nil values, adjust accordingly }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
errors/errors.go(1 hunks)internal/exec/yaml_func_aws_test.go(2 hunks)internal/exec/yaml_func_utils.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
internal/exec/yaml_func_utils.goerrors/errors.gointernal/exec/yaml_func_aws_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Useassert.ErrorIs(t, err, ErrSentinel)for checking our own errors and stdlib errors. Only use string matching for third-party errors
NEVER useassert.Contains(err.Error(), ...)for error checking. ALWAYS useassert.ErrorIs()instead. NEVER use string comparison:err.Error() == "..."orstrings.Contains(err.Error(), ...)
Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoidos.Exit,CheckErrorPrintAndExit, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Usego.uber.org/mock/mockgenwith//go:generatedirectives for mock generation. Never create manual mocks. Pattern://go:generate go run go.uber.org/mock/mockgen@latest -source=filename.go -destination=mock_filename_test.go
Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Files:
internal/exec/yaml_func_aws_test.go
🧠 Learnings (27)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
📚 Learning: 2025-04-24T01:40:13.576Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:40:13.576Z
Learning: When processing YAML nodes with custom directives in Go using gopkg.in/yaml.v3, setting node.Tag = "" is sufficient to prevent re-processing of the node. It's not necessary to also clear node.Value after updating the configuration store (e.g., Viper), as the original value doesn't affect subsequent operations once the tag is removed.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
internal/exec/yaml_func_utils.gointernal/exec/yaml_func_aws_test.go
📚 Learning: 2024-11-30T22:07:08.610Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.
Applied to files:
internal/exec/yaml_func_utils.gointernal/exec/yaml_func_aws_test.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-09-30T19:03:50.738Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
internal/exec/yaml_func_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Use sentinel errors from `errors/errors.go` with ErrorBuilder for all user-facing errors. Use `errors.Is()` for checking errors, never string comparison. `WithCause()` preserves underlying error messages, `WithExplanation()` adds context, `WithHint()` provides guidance, `WithContext()` adds key-value pairs
Applied to files:
errors/errors.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.
Applied to files:
errors/errors.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
internal/exec/yaml_func_aws_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
internal/exec/yaml_func_aws_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
internal/exec/yaml_func_aws_test.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
internal/exec/yaml_func_aws_test.go
🧬 Code graph analysis (2)
internal/exec/yaml_func_utils.go (5)
pkg/schema/schema.go (2)
AtmosConfiguration(53-96)ConfigAndStacksInfo(642-737)internal/exec/yaml_func_resolution_context.go (1)
ResolutionContext(23-26)pkg/utils/yaml_utils.go (11)
AtmosYamlFuncTerraformOutput(26-26)AtmosYamlFuncTerraformState(27-27)AtmosYamlFuncTemplate(25-25)AtmosYamlFuncExec(22-22)AtmosYamlFuncStoreGet(24-24)AtmosYamlFuncStore(23-23)AtmosYamlFuncEnv(28-28)AtmosYamlFuncAwsAccountID(33-33)AtmosYamlFuncAwsCallerIdentityArn(34-34)AtmosYamlFuncAwsCallerIdentityUserID(35-35)AtmosYamlFuncAwsRegion(36-36)pkg/utils/yaml_func_exec.go (1)
ProcessTagExec(11-34)pkg/utils/yaml_func_env.go (1)
ProcessTagEnv(22-80)
internal/exec/yaml_func_aws_test.go (2)
pkg/utils/yaml_utils.go (4)
AtmosYamlFuncAwsAccountID(33-33)AtmosYamlFuncAwsCallerIdentityArn(34-34)AtmosYamlFuncAwsCallerIdentityUserID(35-35)AtmosYamlFuncAwsRegion(36-36)internal/exec/aws_getter.go (1)
ClearAWSIdentityCache(158-164)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/yaml_func_utils.go (4)
61-109: LGTM! Error handling pattern is solid.The refactor correctly implements error propagation with a first-error accumulator and short-circuit logic. Returning the original value on error (line 77) while marking
firstErrallows graceful partial processing, which aligns with the PR's goal of enabling YQ defaults to work when recoverable errors occur.
127-145: Clean error propagation for context-aware tags.The triple-return pattern
(result, handled, error)is idiomatic and makes the control flow clear. Both terraform.output and terraform.state branches correctly propagate errors.
202-215: Error propagation wired correctly.Both helper function calls properly capture and return errors. Unrecognized tags correctly fall through unchanged, which preserves YAML processing semantics.
180-191: AWS functions DO propagate errors—they exit fatally viaCheckErrorPrintAndExit.The review comment incorrectly assumes errors aren't handled. In reality,
processTagAwsValue(yaml_func_aws.go:51) logs errors and callsCheckErrorPrintAndExitwhen AWS identity lookups fail. This is intentional design: AWS authentication failures are fatal and cause process termination, unlike!execand!enverrors which are recoverable and propagated via return values.The distinction is by design: AWS functions represent foundational dependencies (authentication), while
!execand!envrepresent optional operations that can fall back to YQ defaults.Likely an incorrect or invalid review comment.
internal/exec/yaml_func_aws_test.go (1)
486-511: Skip test updates are correct.All four AWS function skip tests properly capture and assert on the error return value. The assertions correctly verify that skipped functions return
handled=falsewith no errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exec/yaml_func_terraform_output_yq_defaults_test.go (2)
96-102: Consider using ErrorIs for sentinel error checking.Line 101 uses string matching to check error messages. Since the mock returns an error wrapping
errUtils.ErrGetObjectFromS3(line 89), consider checking for the sentinel error directly:result, err := ProcessCustomYamlTags(atmosConfig, input, "test-stack", nil, nil) // API errors should propagate even when a YQ default is present. assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "S3 connection timeout") + assert.ErrorIs(t, err, errUtils.ErrGetObjectFromS3)Based on coding guidelines, use ErrorIs for our own sentinel errors rather than string matching.
309-315: Consider using ErrorIs for consistent error checking.Similar to the earlier test, line 314 uses string matching. Consider checking for error types consistently across the test suite:
result, err := ProcessCustomYamlTags(atmosConfig, input, "test-stack", nil, nil) // Without a default, API errors should propagate. assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "S3 connection timeout") + // Check that an error occurred - the mock returns a generic error without wrapping + assert.Error(t, err)Based on coding guidelines, prefer ErrorIs for type checking over string matching.
internal/exec/yaml_func_terraform_state_yq_defaults_test.go (1)
249-255: Use ErrorIs pattern for consistency.Line 254 uses string matching, while other tests in this file (like line 341) correctly use
assert.ErrorIs(). For consistency:result, err := ProcessCustomYamlTags(atmosConfig, input, "test-stack", nil, nil) // API errors should propagate as errors, not use defaults. assert.Error(t, err) assert.Nil(t, result) - assert.Contains(t, err.Error(), "S3 GetObject timeout") + // Error occurred, which is expected for API failures + assert.NotNil(t, err)This aligns with the coding guidelines and matches the pattern used elsewhere in this file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
NOTICE(2 hunks)go.mod(1 hunks)internal/exec/yaml_func_terraform_output_yq_defaults_test.go(1 hunks)internal/exec/yaml_func_terraform_state_yq_defaults_test.go(1 hunks)website/blog/2025-12-03-yq-defaults-yaml-functions.mdx(1 hunks)website/docs/cli/commands/devcontainer/shell.mdx(1 hunks)website/docs/functions/yaml/terraform.output.mdx(2 hunks)website/docs/functions/yaml/terraform.state.mdx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- website/docs/cli/commands/devcontainer/shell.mdx
- NOTICE
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Useassert.ErrorIs(t, err, ErrSentinel)for checking our own errors and stdlib errors. Only use string matching for third-party errors
NEVER useassert.Contains(err.Error(), ...)for error checking. ALWAYS useassert.ErrorIs()instead. NEVER use string comparison:err.Error() == "..."orstrings.Contains(err.Error(), ...)
Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoidos.Exit,CheckErrorPrintAndExit, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Usego.uber.org/mock/mockgenwith//go:generatedirectives for mock generation. Never create manual mocks. Pattern://go:generate go run go.uber.org/mock/mockgen@latest -source=filename.go -destination=mock_filename_test.go
Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in thewebsite/directory when adding new features, ensure consistency between CLI help text and website documentation, and follow the website's documentation structure and style
Keep website code in thewebsite/directory, follow the existing website architecture and style, and test website changes locally before committing
Keep CLI documentation and website documentation in sync and document new features on the website with examples and use cases
Files:
website/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdxwebsite/blog/2025-12-03-yq-defaults-yaml-functions.mdx
{go.mod,go.sum}
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Files:
go.mod
website/blog/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
Blog posts require YAML front matter with slug, title, authors, and tags. Use
<!--truncate-->after intro. ONLY use existing tags fromwebsite/blog/*.mdx- check before writing. Author field must be GitHub username, added towebsite/blog/authors.yml. Filename format:YYYY-MM-DD-feature-name.mdx. PRs labeledminor/majorMUST include blog post
Files:
website/blog/2025-12-03-yq-defaults-yaml-functions.mdx
🧠 Learnings (21)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : New configs support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`. Implement template functions in `internal/exec/template_funcs.go`, register them, add tests, and update documentation
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gowebsite/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdxinternal/exec/yaml_func_terraform_state_yq_defaults_test.gowebsite/blog/2025-12-03-yq-defaults-yaml-functions.mdx
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2025-09-30T19:03:50.738Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 1560
File: pkg/utils/string_utils.go:43-64
Timestamp: 2025-09-30T19:03:50.738Z
Learning: In the Atmos codebase, YAML tags like !terraform.output rely on positional arguments, so the SplitStringByDelimiter function in pkg/utils/string_utils.go must preserve empty strings (even after trimming quotes) to maintain the correct number of positional arguments. Filtering out empty values after trimming would collapse the array and break these function calls.
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gowebsite/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdxinternal/exec/yaml_func_terraform_state_yq_defaults_test.gowebsite/blog/2025-12-03-yq-defaults-yaml-functions.mdx
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to tests/**/*.yaml : NEVER modify `tests/test-cases/` or `tests/testdata/` unless explicitly instructed. Golden snapshots are sensitive to minor changes. Use fixtures in `tests/test-cases/`: `atmos.yaml`, `stacks/`, `components/`
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
internal/exec/yaml_func_terraform_output_yq_defaults_test.gointernal/exec/yaml_func_terraform_state_yq_defaults_test.go
📚 Learning: 2024-12-03T04:01:16.446Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx:0-0
Timestamp: 2024-12-03T04:01:16.446Z
Learning: In the `terraform.output.mdx` documentation file (`website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx`), the cache invalidation and cache scope behavior for the `!terraform.output` function are already described.
Applied to files:
website/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdxwebsite/blog/2025-12-03-yq-defaults-yaml-functions.mdx
📚 Learning: 2024-12-03T03:49:30.395Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx:104-110
Timestamp: 2024-12-03T03:49:30.395Z
Learning: In the documentation for `!terraform.output`, warnings about template variable availability are already covered in other sections, so no need to suggest adding them here.
Applied to files:
website/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdx
📚 Learning: 2024-12-03T05:29:07.718Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/terraform_utils.go:145-146
Timestamp: 2024-12-03T05:29:07.718Z
Learning: In the Atmos project, a 5-minute timeout in the `execTerraformOutput` function is acceptable for retrieving `terraform output` for a component in a stack.
Applied to files:
website/docs/functions/yaml/terraform.output.mdxwebsite/docs/functions/yaml/terraform.state.mdx
📚 Learning: 2025-01-19T22:30:27.600Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-01-19T22:30:27.600Z
Learning: The Atmos YAML function `!env` is used to retrieve environment variables and assign them to sections in stack manifests. It supports both simple types (string, number, boolean) and complex types (JSON-encoded lists, maps, objects).
Applied to files:
website/docs/functions/yaml/terraform.state.mdx
📚 Learning: 2024-10-27T04:34:08.011Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:183-187
Timestamp: 2024-10-27T04:34:08.011Z
Learning: In the `getStackTerraformStateFolder` function, it's acceptable and not an error if no Terraform state folders are found for a stack.
Applied to files:
website/docs/functions/yaml/terraform.state.mdx
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to {go.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Applied to files:
go.mod
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: Go version 1.23.0 was deliberately introduced by the maintainer (aknysh) in January 2025. While this might be a pre-release or development version of Go, it has been approved for use in this project.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: The project uses Go version 1.23.0 which has been confirmed by the maintainer to be working in production for months. Do not flag this as an invalid Go version.
Applied to files:
go.mod
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/yaml_func_terraform_state_yq_defaults_test.go
🧬 Code graph analysis (1)
internal/exec/yaml_func_terraform_state_yq_defaults_test.go (4)
errors/errors.go (4)
ErrTerraformStateNotProvisioned(100-100)ErrTerraformOutputNotFound(101-101)ErrGetObjectFromS3(109-109)ErrTerraformBackendAPIError(105-105)internal/exec/mock_terraform_state_getter.go (1)
NewMockTerraformStateGetter(32-36)pkg/schema/schema.go (2)
AtmosConfiguration(53-96)AtmosSectionMapType(14-14)internal/exec/yaml_func_utils.go (1)
ProcessCustomYamlTags(12-29)
🪛 LanguageTool
website/docs/functions/yaml/terraform.state.mdx
[style] ~528-~528: Using many exclamation marks might seem excessive (in this case: 32 exclamation marks for a text that’s 14386 characters long)
Context: ...sn't exist (e.g., not yet provisioned), !terraform.state returns an error unless...
(EN_EXCESSIVE_EXCLAMATION)
website/blog/2025-12-03-yq-defaults-yaml-functions.mdx
[style] ~13-~13: Consider using a different verb for a more formal wording.
Context: ...te: 2025-12-03T00:00:00.000Z --- We've fixed an issue where YQ default values (using...
(FIX_RESOLVE)
[typographical] ~56-~56: Consider using a typographic close quote here.
Context: ...sn't provisioned, bucket_name will be "default-bucket". ### List Defaults ``...
(EN_QUOTES)
[typographical] ~65-~65: Consider using a typographic opening quote here.
Context: ...nt isn't provisioned, subnets will be ["subnet-a", "subnet-b"]. ### Map Defaul...
(EN_QUOTES)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (9)
internal/exec/yaml_func_terraform_output_yq_defaults_test.go (1)
1-534: Excellent test coverage for YQ default behavior.The test suite comprehensively covers:
- Recoverable vs non-recoverable error scenarios
- Various data types (strings, lists, maps, numbers, booleans)
- Edge cases like nil values and empty strings
- Error propagation paths
This aligns well with the PR's goal of fixing YQ defaults for Terraform YAML functions.
internal/exec/yaml_func_terraform_state_yq_defaults_test.go (2)
16-70: Well-structured helper function tests.The table-driven tests for
isRecoverableTerraformErrorthoroughly cover:
- Direct sentinel errors
- Wrapped sentinel errors
- Non-recoverable errors
- Edge cases (nil)
This testing approach aligns with coding guidelines for table-driven tests.
73-157: Comprehensive YQ expression detection tests.The
hasYqDefaulttests cover diverse YQ syntax patterns including strings, lists, maps, numeric/boolean defaults, empty strings, null, and chained defaults. Good coverage for validating the detection logic.website/blog/2025-12-03-yq-defaults-yaml-functions.mdx (1)
1-101: Well-structured blog post documenting the fix.The blog post effectively:
- Explains the problem users experienced
- Details the solution approach (error classification, YQ evaluation)
- Provides practical examples for different data types
- Includes a helpful error handling behavior table
- Notes that no migration is required
The static analysis hints about style (line 13, 56, 65) are pedantic and don't impact the content quality.
website/docs/functions/yaml/terraform.output.mdx (2)
172-187: Clear documentation of default value behavior.The new "Default Value Behavior" section effectively distinguishes between:
- Recoverable errors (component not provisioned, output missing)
- Non-recoverable errors (API failures)
- How defaults interact with each scenario
This provides users with clear guidance on error handling expectations.
493-493: Updated cold-start guidance aligns with new behavior.The consideration now correctly references that errors occur for unprovisioned components unless a default is provided, with a link to detailed error handling documentation. This is consistent with the new error propagation model.
website/docs/functions/yaml/terraform.state.mdx (2)
167-182: Consistent default behavior documentation.The updated documentation parallels the terraform.output changes while appropriately adapting the examples for backend-specific errors (S3 access denied, GCS timeouts, Azure connectivity). This maintains consistency across the YAML function documentation.
528-528: Cold-start consideration updated appropriately.The consideration now references that state file absence returns an error unless a YQ default is provided, linking to the detailed error handling section. This aligns with the new behavior model.
The static analysis hint about exclamation marks is pedantic - the YAML function syntax uses
!as part of the tag notation, which is required and not excessive.go.mod (1)
24-24: Use a valid AWS SDK for Go v2 version; v1.41.0 does not appear to exist in official releases.The version v1.41.0 for
github.com/aws/aws-sdk-go-v2could not be found in the official AWS SDK for Go v2 releases. The latest documented core module version as of December 2025 is v1.40.0 (released 2025-11-19, which added AWS Login credentials support). Confirm the intended version—either use v1.40.0 or verify if a pre-release/development version is intentional.⛔ Skipped due to learnings
Learnt from: aknysh Repo: cloudposse/atmos PR: 1363 File: internal/exec/template_utils.go:18-18 Timestamp: 2025-07-05T20:59:02.914Z Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.Learnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to {go.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependenciesLearnt from: osterman Repo: cloudposse/atmos PR: 808 File: pkg/config/config.go:478-483 Timestamp: 2024-12-02T21:26:32.337Z Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.Learnt from: aknysh Repo: cloudposse/atmos PR: 944 File: go.mod:3-3 Timestamp: 2025-01-17T00:21:32.987Z Learning: The project uses Go version 1.23.0 which has been confirmed by the maintainer to be working in production for months. Do not flag this as an invalid Go version.Learnt from: aknysh Repo: cloudposse/atmos PR: 944 File: go.mod:3-3 Timestamp: 2025-01-17T00:21:32.987Z Learning: Go version 1.23.0 was deliberately introduced by the maintainer (aknysh) in January 2025. While this might be a pre-release or development version of Go, it has been approved for use in this project.
|
Closes #1830 |
2 participants
what
//fallback operator) now work correctly with!terraform.stateand!terraform.outputYAML functions when components are not provisioned or outputs don't exist(any, error)tuples instead of callingos.Exit()viaCheckErrorPrintAndExit//fallback operatorwhy
Users experienced sporadic failures with YQ expression defaults in
!terraform.stateand!terraform.outputfunctions. Root cause: YAML function wrappers returned/exited before YQ pipeline could evaluate defaults. The fix ensures YQ defaults are evaluated for missing data while still failing on API errors that shouldn't use fallbacks.references
Fixes sporadic failures with YQ default expressions reported by users. Implementation eliminates deep exit calls in favor of proper error propagation through the call stack.
Summary by CodeRabbit
New Features
terraform.outputandterraform.statefunctions when components aren't provisioned or outputs are missing.Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.