fix: prevent YAML function concatenation in lists#1506
Conversation
When YAML functions (like !terraform.state or !terraform.output) are used in lists, they were keeping their custom tags after being processed, which could cause issues during YAML decoding. This fix clears the custom tag after incorporating it into the node value, preventing potential double processing or concatenation issues. The fix ensures that: - Each list item with a YAML function remains separate - Functions can return any type (string, map, list, etc.) - No "invalid number of arguments" errors occur from concatenation Added comprehensive tests to verify: - YAML functions work correctly in lists - Different return types are handled properly - No concatenation of multiple functions occurs - Mixed lists with functions and static values work Fixes issue where users reported: "invalid number of arguments in the Atmos YAML function !terraform.state image1-ecr global repository_arn !terraform.state image2-ecr global repository_arn" 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds tests validating Atmos YAML custom function handling (especially !terraform.state and !terraform.output) in lists, maps, and nested structures, prevents YAML decoder re-processing of custom tags by clearing node tags after value computation, adds Terraform test fixtures and a scenario, and improves URL handling in template tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Tests/CLI
participant Decoder as YAML Decoder
participant TagEval as getValueWithTag
participant Post as processCustomTags
Test->>Decoder: Load & unmarshal YAML (includes custom tags)
Decoder->>TagEval: Encounter custom tag (!terraform.state/!terraform.output)
TagEval-->>Decoder: Return computed value
Note right of TagEval #f8f0d0: Clear node.Tag to avoid re-processing
Decoder-->>Test: Return decoded structure (values preserved)
Test->>Post: Post-process decoded structure to finalize functions
Post-->>Test: Resolved values/types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1506 +/- ##
==========================================
+ Coverage 57.02% 57.07% +0.04%
==========================================
Files 286 286
Lines 30587 30592 +5
==========================================
+ Hits 17443 17461 +18
+ Misses 11305 11290 -15
- Partials 1839 1841 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Refactored the test fixtures to follow the established pattern of keeping reusable components separate from scenarios: - Created a generic yaml-function-test component in the shared fixtures - Removed scenario-specific component implementation - Updated test stacks to use the reusable component - Maintained comprehensive test coverage for all data types The new yaml-function-test component: - Tests scalars (string, number, boolean) - Tests collections (lists, maps) - Tests complex nested structures - Doesn't impose cloud provider conventions - Provides comprehensive outputs for validation This follows the established pattern and makes the test components more maintainable and reusable across different test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/fixtures/components/terraform/yaml-function-test/variables.tf (1)
37-41: Type doesn’t match intent: allow truly mixed types.The description says the list can contain different types, but
list(string)restricts it. Preferlist(any)to align with test intent.Apply:
variable "mixed_type_list" { description = "List that can contain different types (as strings)" - type = list(string) + type = list(any) default = [] }tests/fixtures/components/terraform/yaml-function-test/main.tf (1)
11-15: Locals are unused; consider exposing or removing.Either add outputs for
simulated_string,simulated_list,simulated_mapor drop the locals to reduce noise.For example, in outputs.tf:
+output "simulated_string" { + description = "Derived string via locals" + value = local.simulated_string +} + +output "simulated_list" { + description = "Derived list via locals" + value = local.simulated_list +} + +output "simulated_map" { + description = "Derived map via locals" + value = local.simulated_map +}
📜 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 (5)
tests/fixtures/components/terraform/yaml-function-test/main.tf(1 hunks)tests/fixtures/components/terraform/yaml-function-test/outputs.tf(1 hunks)tests/fixtures/components/terraform/yaml-function-test/variables.tf(1 hunks)tests/fixtures/scenarios/yaml-functions-in-lists/atmos.yaml(1 hunks)tests/fixtures/scenarios/yaml-functions-in-lists/stacks/test-yaml-functions.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/scenarios/yaml-functions-in-lists/stacks/test-yaml-functions.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fixtures/scenarios/yaml-functions-in-lists/atmos.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-01T00:33:20.298Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
tests/fixtures/components/terraform/yaml-function-test/outputs.tftests/fixtures/components/terraform/yaml-function-test/main.tf
📚 Learning: 2024-12-03T04:01:16.446Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
tests/fixtures/components/terraform/yaml-function-test/outputs.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (5)
tests/fixtures/components/terraform/yaml-function-test/variables.tf (2)
1-16: Solid, comprehensive variable coverage.Definitions look consistent and useful for exercising many scenarios.
Also applies to: 18-36, 43-55, 56-74, 75-81
82-99: Widen YAML function variables to accept any type
Apply to tests/fixtures/components/terraform/yaml-function-test/variables.tf:@@ variable "function_results_list" { - type = list(string) + type = list(any) @@ variable "mixed_function_list" { - type = list(string) + type = list(any) @@ variable "function_map_results" { - type = map(string) + type = map(any)tests/fixtures/components/terraform/yaml-function-test/main.tf (1)
5-7: Terraform version constraint is fine.
>= 1.0is broadly compatible for test fixtures.tests/fixtures/components/terraform/yaml-function-test/outputs.tf (2)
1-13: Outputs look consistent and helpful for tests.Covers scalar, list, map, nested, and helper summary outputs.
Also applies to: 14-18, 19-34, 35-45, 46-56, 57-72, 73-91, 92-100
97-100: Conditional guard is good; keep types flexible.If
function_results_listbecomeslist(any), this ternary still works. No change needed.If you see type-unification warnings from Terraform here after widening types, we can add an explicit
type = anyto this output.
- Parse file URLs using url.Parse() instead of string manipulation - Handle Windows-specific path formats (e.g., /D:/temp/file.json) - Remove leading slash from Windows absolute paths after URL parsing - Fixes test failures on Windows CI platform
aknysh
added
no-release
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
internal/exec/template_utils_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
internal/exec/template_utils_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
internal/exec/template_utils_test.go
internal/exec/**.go
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer extending existing utilities in internal/exec/ and pkg/ over creating new ones—search for existing methods before implementation
Files:
internal/exec/template_utils_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: aknysh
PR: cloudposse/atmos#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.
📚 Learning: 2024-10-23T20:13:23.054Z
Learnt from: osterman
PR: cloudposse/atmos#731
File: pkg/utils/file_utils.go:198-202
Timestamp: 2024-10-23T20:13:23.054Z
Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
Applied to files:
internal/exec/template_utils_test.go
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
PR: cloudposse/atmos#887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
internal/exec/template_utils_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
- Use filepath.FromSlash() to convert URL paths to OS-specific format - Handle Windows drive letters correctly (remove leading slash from /C:/) - Convert forward slashes to backslashes on Windows - Fixes test failures on Windows CI where paths were incorrectly formatted
- Handle both file://C:/path and file:///C:/path URL formats on Windows - Safely check for drive letters to avoid index out of bounds - Combine Host and Path when needed for proper Windows file paths - Fixes test failures on Windows where drive letters were missing
…se/atmos into yaml-functions-in-lists
|
These changes were released in v1.192.0. |
|
These changes were released in v1.193.0-test.1. |
2 participants
what
!terraform.stateand!terraform.output) in lists could potentially cause concatenation errorswhy
Users reported an error when using YAML functions in lists:
The issue occurred because custom YAML tags were being incorporated into node values but the tags themselves weren't being cleared. This could lead to unexpected behavior during YAML decoding, especially when functions appeared in lists.
The fix ensures:
references
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests