refactor: Create pkg/function and pkg/yaml packages#1886
Conversation
…tion registry ## Summary Create new packages for modularized function handling and YAML utilities: - **pkg/function/**: Format-agnostic function registry (handlers work across YAML, HCL, JSON) - Function interfaces and base types - Thread-safe registry with Get/Register/GetByPhase operations - Function handlers: env, exec, random, include, template, store, terraform, aws, repo-root - Cycle detection context for dependency resolution - **pkg/yaml/**: YAML-specific utilities (parsing, position tracking, error handling) - Type definitions (LongString, YAMLOptions) - YAML position and range tracking - Error types for YAML processing - **pkg/aws/identity/**: Consolidated AWS identity caching - Moved from internal/aws_utils and internal/exec/aws_getter.go - Thread-safe CallerIdentity caching with auth context support - AWS config loading with role assumption ## Why - **Separation of Concerns**: Function handlers are format-agnostic; only YAML/HCL/JSON parsing differs - **Code Reuse**: Single registry used across all formats, reducing duplication - **Extensibility**: Registry pattern enables new functions without modifying core logic - **Testing**: Interface-driven design with dependency injection for testability - **AWS Consolidation**: Eliminate duplicate AWS identity caching logic ## References Preparation for PR #1842 which refactors YAML processing to use pkg/function registry. Consolidates AWS identity handling per request to eliminate internal/aws_utils duplication. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR refactors AWS utilities and introduces a comprehensive format-agnostic function registry system. It removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
|
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. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Add LiteralFunction handler that preserves values exactly as written,
bypassing template processing. This enables passing template-like syntax
({{...}}, ${...}) to downstream tools like Terraform, Helm, and ArgoCD.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1886 +/- ##
==========================================
+ Coverage 73.23% 73.50% +0.27%
==========================================
Files 614 633 +19
Lines 57827 58652 +825
==========================================
+ Hits 42352 43115 +763
- Misses 12523 12571 +48
- Partials 2952 2966 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/yaml_func_aws_test.go (1)
618-638: Unsafe assertions inside goroutines.
assert.NoErrorandassert.Equalinside goroutines won't properly fail the test—they may panic after the test completes or report to the wrongtesting.T. Collect results or use a mutex-protected error slice.const numGoroutines = 50 - done := make(chan bool, numGoroutines) + results := make(chan error, numGoroutines) for i := 0; i < numGoroutines; i++ { go func() { identity, err := getAWSCallerIdentityCached(ctx, atmosConfig, nil) - assert.NoError(t, err) - assert.Equal(t, "777777777777", identity.Account) - done <- true + if err != nil { + results <- err + return + } + if identity.Account != "777777777777" { + results <- errors.New("unexpected account") + return + } + results <- nil }() } for i := 0; i < numGoroutines; i++ { - <-done + if err := <-results; err != nil { + t.Errorf("goroutine failed: %v", err) + } }
🧹 Nitpick comments (15)
pkg/function/phase.go (1)
21-31: Removeperf.Trackfrom this trivial accessor.
Phase.String()is a simple switch returning a constant string. The tracking overhead exceeds the method cost here. Based on learnings, trivial accessors and hot-path methods should avoidperf.Track().func (p Phase) String() string { - defer perf.Track(nil, "function.Phase.String")() - switch p { case PreMerge: return "pre-merge"If
perf.Trackis removed entirely, theperfimport can also be dropped.pkg/function/context.go (1)
42-66: Consider removingperf.Trackfrom these trivial fluent methods.
WithFile,WithBaseDir, andWithStackInfoare simple copy-and-set operations. Per the project's learnings, tracking overhead on such trivial methods provides no actionable performance data. These may be called frequently during YAML processing.func (ctx *ExecutionContext) WithFile(file string) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithFile")() - newCtx := *ctx newCtx.File = file return &newCtx } func (ctx *ExecutionContext) WithBaseDir(baseDir string) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithBaseDir")() - newCtx := *ctx newCtx.BaseDir = baseDir return &newCtx } func (ctx *ExecutionContext) WithStackInfo(stackInfo *schema.ConfigAndStacksInfo) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithStackInfo")() - newCtx := *ctx newCtx.StackInfo = stackInfo return &newCtx }pkg/aws/identity/identity_test.go (1)
159-171: Consider removing this tautological test.This test simply verifies that struct field assignments work, which is guaranteed by Go. It doesn't test any behavior or logic. As per coding guidelines, avoid tautological tests that don't add value.
pkg/function/exec.go (1)
49-49: Consider using ExecutionContext environment.The function passes
os.Environ()directly, which doesn't include the component'senvsection from stack manifests. TheEnvFunctionchecksexecCtx.StackInfo.GetComponentEnvSection()first to respect stack-specific environment variables. Consider merging or prioritizing the execution context environment if provided.- res, err := utils.ExecuteShellAndReturnOutput(args, YAMLTag(TagExec)+" "+args, ".", os.Environ(), false) + // Merge component env section with os.Environ() if available. + env := os.Environ() + if execCtx != nil && execCtx.StackInfo != nil { + if envSection := execCtx.StackInfo.GetComponentEnvSection(); envSection != nil { + for k, v := range envSection { + env = append(env, fmt.Sprintf("%s=%v", k, v)) + } + } + } + res, err := utils.ExecuteShellAndReturnOutput(args, YAMLTag(TagExec)+" "+args, ".", env, false)pkg/function/terraform.go (1)
3-11: Imports should have blank line separators between groups.Per coding guidelines, imports should be organized in three groups separated by blank lines: stdlib, 3rd-party, Atmos packages.
import ( "context" "fmt" "strings" - + log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/utils" )pkg/yaml/position.go (2)
3-7: Import order should be: stdlib, 3rd-party, Atmos packages.import ( + goyaml "gopkg.in/yaml.v3" + "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/utils" - goyaml "gopkg.in/yaml.v3" )
62-67: Simplify path building using AppendJSONPathKey.The utility function already handles empty base paths correctly.
- var path string - if currentPath == "" { - path = key - } else { - path = utils.AppendJSONPathKey(currentPath, key) - } + path := utils.AppendJSONPathKey(currentPath, key)pkg/yaml/errors.go (1)
5-23: Keep errors local if this follows the established package-specific pattern; otherwise centralize to errors/errors.go.Per coding guidelines, errors should be centralized in
errors/errors.go. However, if pkg/yaml follows a pattern similar to toolchain-specific packages (where canonical sentinels live in the package with re-exports), this is acceptable. If moving these to the central file, ensure no import cycles are introduced with pkg/schema or other dependencies. This can be addressed in a separate refactor PR if deferring changes.pkg/function/random.go (1)
65-68: Unreachable case 0 branch.The
case 0block is unreachable—args == ""is already handled at lines 54-56 before splitting.SplitStringByDelimiterfilters empty strings, solen(parts)won't be 0 for non-empty input.switch len(parts) { - case 0: - min = defaultRandomMin - max = defaultRandomMax - case 1:pkg/function/store.go (1)
61-66: Inconsistent nil-value handling compared to StoreGetFunction.
StoreFunction.Executedoesn't check ifvalue == nilbefore returning (only handles errors), butStoreGetFunction.Executeat lines 134-136 explicitly checks fornilvalue and applies the default. Consider aligning behavior.value, err := store.Get(params.stack, params.component, params.key) if err != nil { if params.defaultValue != nil { return *params.defaultValue, nil } return nil, fmt.Errorf("%w: failed to get key '%s': %w", ErrExecutionFailed, params.key, err) } + + // Check if the retrieved value is nil and use default if provided. + if value == nil && params.defaultValue != nil { + return *params.defaultValue, nil + }pkg/function/tags.go (1)
63-105:AllTags()andTagsMapcan drift out of sync.Both list the same tags but are maintained separately. If a new tag is added to one but not the other, bugs ensue. Consider generating
TagsMapfromAllTags()or using a single source of truth.One approach—compute TagsMap at init time:
var TagsMap map[string]bool func init() { TagsMap = make(map[string]bool, len(allTagsList)) for _, tag := range allTagsList { TagsMap[tag] = true } }pkg/function/resolution/context.go (1)
168-193: Error message includes sentinel twice.Line 172 embeds
errUtils.ErrCircularDependencyas a string in the message, then line 192 wraps it again with%w. This duplicates the sentinel text in the output.- builder.WriteString(fmt.Sprintf("%s\n\n", errUtils.ErrCircularDependency)) + builder.WriteString("Circular dependency detected\n\n") builder.WriteString("Dependency chain:\n")This keeps the message clean while still allowing
errors.Is()checks via the wrap at line 192.pkg/function/registry.go (1)
164-185: Unregister only works with primary name, not aliases.If someone calls
Unregister("some-alias"), it won't find the function since it looks inr.functionsdirectly. Consider resolving aliases first.func (r *Registry) Unregister(name string) { defer perf.Track(nil, "function.Registry.Unregister")() r.mu.Lock() defer r.mu.Unlock() name = strings.ToLower(name) + // Resolve alias to primary name if needed. + if primaryName, exists := r.aliases[name]; exists { + name = primaryName + } + fn, exists := r.functions[name] if !exists { return }pkg/aws/identity/identity.go (1)
56-66: Redundant struct copy.
GetCallerIdentityalready returns*CallerIdentity, so the field-by-field copy is unnecessary.// Use the exported function to get caller identity. result, err := GetCallerIdentity(ctx, "", "", 0, authContext) if err != nil { return nil, err } - identity := &CallerIdentity{ - Account: result.Account, - Arn: result.Arn, - UserID: result.UserID, - Region: result.Region, - } - log.Debug("Retrieved AWS caller identity", - "account", identity.Account, - "arn", identity.Arn, - "user_id", identity.UserID, - "region", identity.Region, + "account", result.Account, + "arn", result.Arn, + "user_id", result.UserID, + "region", result.Region, ) - return identity, nil + return result, nilpkg/function/aws.go (1)
144-179: LGTM, with an optional DRY observation.All four Execute methods share nearly identical structure. Could consolidate with a field-selector approach if you want to reduce duplication in the future, but the current explicit implementation is perfectly readable.
If desired, you could consolidate the four handlers using a field extractor:
type awsIdentityFieldFunc func(*awsIdentity.CallerIdentity) string func (f *BaseFunction) executeAWSField(ctx context.Context, execCtx *ExecutionContext, extract awsIdentityFieldFunc) (any, error) { identity, err := getAWSIdentity(ctx, execCtx) if err != nil { log.Error("Failed to get AWS caller identity", "error", err) return nil, err } return extract(identity), nil }Then each Execute becomes a one-liner. Low priority though - current approach is clear.
📜 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 (35)
.golangci.yml(1 hunks)internal/aws_utils/aws_utils.go(0 hunks)internal/aws_utils/aws_utils_test.go(0 hunks)internal/exec/aws_getter.go(2 hunks)internal/exec/yaml_func_aws_test.go(3 hunks)internal/terraform_backend/terraform_backend_s3.go(2 hunks)pkg/aws/identity/doc.go(1 hunks)pkg/aws/identity/identity.go(1 hunks)pkg/aws/identity/identity_test.go(1 hunks)pkg/function/aws.go(1 hunks)pkg/function/context.go(1 hunks)pkg/function/defaults.go(1 hunks)pkg/function/doc.go(1 hunks)pkg/function/env.go(1 hunks)pkg/function/errors.go(1 hunks)pkg/function/exec.go(1 hunks)pkg/function/function.go(1 hunks)pkg/function/git_root.go(1 hunks)pkg/function/include.go(1 hunks)pkg/function/literal.go(1 hunks)pkg/function/phase.go(1 hunks)pkg/function/random.go(1 hunks)pkg/function/registry.go(1 hunks)pkg/function/registry_test.go(1 hunks)pkg/function/resolution/context.go(1 hunks)pkg/function/resolution/errors.go(1 hunks)pkg/function/store.go(1 hunks)pkg/function/tags.go(1 hunks)pkg/function/template.go(1 hunks)pkg/function/terraform.go(1 hunks)pkg/provisioner/backend/s3.go(2 hunks)pkg/yaml/doc.go(1 hunks)pkg/yaml/errors.go(1 hunks)pkg/yaml/position.go(1 hunks)pkg/yaml/types.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/aws_utils/aws_utils_test.go
- internal/aws_utils/aws_utils.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: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in 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 for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/function/context.gointernal/terraform_backend/terraform_backend_s3.gopkg/function/resolution/errors.gopkg/provisioner/backend/s3.gopkg/function/defaults.gopkg/function/include.gopkg/function/errors.gopkg/function/git_root.gopkg/yaml/doc.gopkg/function/env.gopkg/aws/identity/identity_test.gopkg/yaml/types.gopkg/function/phase.gopkg/yaml/position.gopkg/function/terraform.gopkg/function/registry_test.gopkg/function/store.gopkg/function/tags.gointernal/exec/aws_getter.gopkg/function/doc.gopkg/aws/identity/doc.gopkg/function/exec.gointernal/exec/yaml_func_aws_test.gopkg/function/random.gopkg/function/template.gopkg/yaml/errors.gopkg/function/function.gopkg/aws/identity/identity.gopkg/function/registry.gopkg/function/resolution/context.gopkg/function/literal.gopkg/function/aws.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: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/aws/identity/identity_test.gopkg/function/registry_test.gointernal/exec/yaml_func_aws_test.go
.golangci.yml
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Configure golangci-lint in
.golangci.ymlwith specific linters enabled: gofmt, goimports, govet, staticcheck, errcheck, ineffassign, misspell, unused, revive, gocritic
Files:
.golangci.yml
🧠 Learnings (67)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
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: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Avoid utils package bloat; don't add new functions to pkg/utils/; create purpose-built packages instead (examples: pkg/store/, pkg/git/, pkg/pro/, pkg/filesystem/)
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
pkg/function/context.gopkg/provisioner/backend/s3.gopkg/function/env.gopkg/function/phase.gopkg/yaml/position.gopkg/function/terraform.gopkg/function/store.gopkg/function/tags.gopkg/function/doc.gopkg/yaml/errors.gopkg/function/function.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:
pkg/function/context.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
internal/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/aws/identity/doc.gopkg/aws/identity/identity.go.golangci.ymlpkg/function/aws.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/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/function/tags.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/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Organize imports in 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/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/function/tags.gopkg/yaml/errors.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
internal/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.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/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/function/phase.gopkg/function/tags.go
📚 Learning: 2025-05-30T03:21:37.197Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1274
File: go.mod:63-63
Timestamp: 2025-05-30T03:21:37.197Z
Learning: The redis dependency (github.com/redis/go-redis/v9) in the atmos project is only used in tests, not in production code.
Applied to files:
internal/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.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/terraform_backend/terraform_backend_s3.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
internal/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/yaml/errors.go
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
internal/terraform_backend/terraform_backend_s3.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/terraform_backend/terraform_backend_s3.gopkg/provisioner/backend/s3.gopkg/yaml/errors.gopkg/function/function.go
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
pkg/function/resolution/errors.gopkg/function/errors.go
📚 Learning: 2025-11-10T20:03:56.875Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1775
File: pkg/auth/providers/aws/sso_provisioning.go:40-79
Timestamp: 2025-11-10T20:03:56.875Z
Learning: In the Atmos AWS SSO provider (pkg/auth/providers/aws/sso_provisioning.go), the OAuth access token from the AWS SSO device flow is intentionally stored in the `AccessKeyID` field of `AWSCredentials` during authentication. This token is then extracted and used for ListAccounts and ListAccountRoles API calls during identity provisioning. This design reuses the existing `AWSCredentials` type for token transport rather than creating a separate credential type.
Applied to files:
pkg/provisioner/backend/s3.gopkg/aws/identity/identity.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
pkg/provisioner/backend/s3.go
📚 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 `!include` allows downloading local or remote files from different sources and assigning their contents to sections in stack manifests. It supports various protocols (file, http, git, s3, etc.) and can filter content using YQ expressions.
Applied to files:
pkg/function/include.gopkg/yaml/errors.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:
pkg/function/include.gopkg/yaml/doc.gopkg/yaml/errors.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Applied to files:
pkg/function/errors.gopkg/yaml/errors.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:
pkg/function/errors.gopkg/yaml/doc.gopkg/yaml/errors.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 : Document all exported functions, types, and methods following Go's documentation conventions
Applied to files:
pkg/yaml/doc.gopkg/function/doc.go
📚 Learning: 2025-04-24T01:06:15.259Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:06:15.259Z
Learning: When processing YAML structures with gopkg.in/yaml.v3, the Value field of a Node is only meant for scalar values. Complex data types like maps and arrays are represented through child nodes in the YAML tree structure.
Applied to files:
pkg/yaml/doc.gopkg/yaml/position.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/yaml/doc.gopkg/yaml/position.gopkg/function/terraform.gopkg/function/tags.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
pkg/yaml/doc.gopkg/yaml/errors.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:
pkg/yaml/doc.go
📚 Learning: 2024-12-12T17:13:53.409Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 848
File: pkg/utils/doc_utils.go:19-22
Timestamp: 2024-12-12T17:13:53.409Z
Learning: In `pkg/utils/doc_utils.go`, the `DisplayDocs` function uses the `PAGER` environment variable, which is intentionally user-configurable to allow users to specify custom pager commands that fit their workflow; adding validation to restrict it is not desired.
Applied to files:
pkg/yaml/doc.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/function/env.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*.go : Use ATMOS_ prefix for environment variables with viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")
Applied to files:
pkg/function/env.go
📚 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:
pkg/function/env.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 : Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Applied to files:
pkg/function/env.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
pkg/function/env.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/function/env.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/aws/identity/identity_test.gopkg/function/registry_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
pkg/aws/identity/identity_test.gopkg/function/registry_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:
pkg/aws/identity/identity_test.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/aws/identity/identity_test.gopkg/function/registry_test.gointernal/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 : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/aws/identity/identity_test.gopkg/function/registry_test.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:
pkg/function/phase.gopkg/function/tags.go
📚 Learning: 2025-10-16T05:26:31.892Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1639
File: pkg/merge/merge.go:392-430
Timestamp: 2025-10-16T05:26:31.892Z
Learning: In Atmos merge operations (pkg/merge/merge.go), context.Positions is a PositionMap (map[string]*schema.Position) keyed by JSONPath strings, not a slice indexed by input array position. Position lookups are path-based, making them independent of input array reorganization or filtering. This design allows filtering empty inputs without affecting provenance tracking correctness.
Applied to files:
pkg/yaml/position.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:
pkg/function/terraform.gopkg/function/tags.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
pkg/function/terraform.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
pkg/function/terraform.gopkg/function/tags.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:
pkg/function/terraform.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
pkg/function/terraform.gointernal/exec/yaml_func_aws_test.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:
pkg/function/terraform.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
pkg/function/terraform.gopkg/function/tags.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:
pkg/function/terraform.gopkg/function/tags.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
pkg/function/terraform.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:
pkg/function/terraform.go
📚 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:
pkg/function/terraform.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:
pkg/function/terraform.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/function/registry_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:
pkg/function/registry_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/function/registry_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:
pkg/function/registry_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:
pkg/function/registry_test.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:
pkg/function/tags.gopkg/yaml/errors.go
📚 Learning: 2025-12-16T18:20:55.614Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.614Z
Learning: New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
pkg/function/tags.gopkg/function/template.go
📚 Learning: 2025-01-25T03:49:03.951Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/template_utils.go:268-271
Timestamp: 2025-01-25T03:49:03.951Z
Learning: The `ProcessTmplWithDatasourcesGomplate` function in `internal/exec/template_utils.go` is used for documentation generation purposes, where simple environment variable handling is acceptable and thread-safety concerns are not critical.
Applied to files:
pkg/function/template.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:
pkg/yaml/errors.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 : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/yaml/errors.go
📚 Learning: 2024-12-12T15:17:45.245Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
Applied to files:
pkg/yaml/errors.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/yaml/errors.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
pkg/yaml/errors.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.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Applied to files:
pkg/function/resolution/context.go.golangci.yml
📚 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 : All code must pass golangci-lint checks
Applied to files:
.golangci.yml
🧬 Code graph analysis (18)
pkg/function/context.go (2)
pkg/schema/schema.go (2)
AtmosConfiguration(54-99)ConfigAndStacksInfo(667-763)pkg/perf/perf.go (1)
Track(121-138)
internal/terraform_backend/terraform_backend_s3.go (1)
pkg/aws/identity/identity.go (1)
LoadConfigWithAuth(251-317)
pkg/provisioner/backend/s3.go (1)
pkg/aws/identity/identity.go (1)
LoadConfigWithAuth(251-317)
pkg/function/git_root.go (7)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (1)
TagRepoRoot(39-39)pkg/function/phase.go (1)
PreMerge(13-13)pkg/function/resolution/context.go (1)
Context(23-26)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/logger/log.go (1)
Debug(24-26)
pkg/aws/identity/identity_test.go (3)
pkg/aws/identity/identity.go (5)
CallerIdentity(21-26)GetCallerIdentity(178-216)ClearIdentityCache(166-172)SetGetter(84-92)GetCallerIdentityCached(124-162)pkg/schema/schema.go (2)
AtmosConfiguration(54-99)AWSAuthContext(629-644)pkg/schema/schema_auth.go (1)
Account(75-78)
pkg/yaml/types.go (1)
pkg/perf/perf.go (1)
Track(121-138)
pkg/function/phase.go (1)
pkg/perf/perf.go (1)
Track(121-138)
pkg/yaml/position.go (2)
pkg/perf/perf.go (1)
Track(121-138)pkg/utils/jsonpath.go (2)
AppendJSONPathKey(50-58)AppendJSONPathIndex(65-71)
internal/exec/aws_getter.go (1)
pkg/aws/identity/identity.go (5)
CallerIdentity(21-26)Getter(32-40)SetGetter(84-92)GetCallerIdentityCached(124-162)ClearIdentityCache(166-172)
pkg/function/exec.go (9)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (2)
TagExec(12-12)YAMLTag(108-112)pkg/function/phase.go (1)
PreMerge(13-13)pkg/function/resolution/context.go (1)
Context(23-26)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/logger/log.go (1)
Debug(24-26)pkg/function/errors.go (1)
ErrInvalidArguments(14-14)pkg/utils/shell_utils.go (1)
ExecuteShellAndReturnOutput(33-62)
pkg/function/random.go (9)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (1)
TagRandom(42-42)pkg/function/phase.go (1)
PreMerge(13-13)pkg/function/resolution/context.go (1)
Context(23-26)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/logger/log.go (1)
Debug(24-26)pkg/utils/string_utils.go (1)
SplitStringByDelimiter(33-66)pkg/function/errors.go (1)
ErrInvalidArguments(14-14)
pkg/function/template.go (6)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (1)
TagTemplate(21-21)pkg/function/phase.go (1)
PreMerge(13-13)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/logger/log.go (1)
Debug(24-26)
pkg/function/function.go (3)
pkg/function/phase.go (1)
Phase(8-8)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/perf/perf.go (1)
Track(121-138)
pkg/aws/identity/identity.go (3)
pkg/schema/schema.go (2)
AtmosConfiguration(54-99)AWSAuthContext(629-644)pkg/logger/log.go (1)
Debug(24-26)errors/errors.go (1)
ErrAwsGetCallerIdentity(95-95)
pkg/function/registry.go (4)
pkg/function/function.go (1)
Function(11-23)pkg/perf/perf.go (1)
Track(121-138)pkg/function/errors.go (2)
ErrFunctionAlreadyRegistered(11-11)ErrFunctionNotFound(7-7)pkg/function/phase.go (1)
Phase(8-8)
pkg/function/resolution/context.go (2)
pkg/perf/perf.go (1)
Track(121-138)pkg/function/resolution/errors.go (1)
ErrCircularDependency(6-6)
pkg/function/literal.go (5)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (1)
TagLiteral(45-45)pkg/function/phase.go (1)
PreMerge(13-13)pkg/logger/log.go (1)
Debug(24-26)
pkg/function/aws.go (7)
pkg/function/context.go (1)
ExecutionContext(10-28)pkg/aws/identity/identity.go (2)
CallerIdentity(21-26)GetCallerIdentityCached(124-162)pkg/perf/perf.go (1)
Track(121-138)pkg/schema/schema.go (3)
AWSAuthContext(629-644)AuthContext(615-625)AtmosConfiguration(54-99)pkg/function/tags.go (4)
TagAwsAccountID(48-48)TagAwsCallerIdentityArn(51-51)TagAwsCallerIdentityUserID(54-54)TagAwsRegion(57-57)pkg/function/phase.go (1)
PostMerge(17-17)pkg/logger/log.go (1)
Debug(24-26)
⏰ 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
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
- resolution/context.go: Fix "false sharing" terminology to "metric collisions" and generate unique fallback IDs (unknown-N) using atomic counter to avoid collisions when multiple goroutines fail goroutine ID parsing - store.go: Use strings.SplitN instead of strings.Fields to handle query values containing spaces (e.g., query ".foo .bar") 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add tests for pkg/function, pkg/yaml, and pkg/aws/identity packages: - pkg/function/store_test.go: Tests for store and store.get functions - pkg/function/aws_test.go: Tests for AWS identity functions - pkg/function/terraform_test.go: Tests for terraform.output/state - pkg/function/template_test.go: Tests for template processing - pkg/function/tags_test.go: Tests for tag constants and helpers - pkg/function/context_test.go: Tests for ExecutionContext - pkg/function/resolution/context_test.go: Tests for circular dependency detection - pkg/yaml/position_test.go: Tests for YAML position extraction - pkg/aws/identity/identity_test.go: Additional cache and concurrency tests Also fix lint issues: - Extract duplicate string constant in aws.go - Remove unused nolint directive in git_root.go Coverage improvements: - pkg/function: 82.4% - pkg/function/resolution: 97.2% - pkg/yaml: 88.6% 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test isolation fix: Tests that redirect os.Stdout/os.Stderr were not properly restoring the original streams when t.Fatalf() was called on failure. This caused subsequent tests to have stdout/stderr pointing to closed pipes, resulting in "stdout not writable" errors in CI. The fix adds defer statements to ensure stdout/stderr are always restored even if tests fail early via t.Fatalf(). Files fixed for test isolation: - internal/exec/terraform_test.go (6 instances) - internal/exec/packer_test.go (3 instances) - internal/exec/terraform_utils_test.go (2 instances) - internal/exec/workflow_adapters_test.go (3 instances) Lint fixes: - pkg/config/load.go: Use errors.As() instead of type switch for error checking - pkg/function/env.go: Reduce cyclomatic complexity by extracting parseEnvArgs and lookupEnvFromContext helpers - pkg/function/store.go: Reduce cyclomatic complexity by extracting retrieveFromStore helper - pkg/function/template.go: Reduce cyclomatic complexity by extracting processTemplateString helper 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Resolve merge conflicts by combining main's concurrent pipe reading (to avoid deadlock when output exceeds pipe buffer) with defer-based cleanup (to ensure stdout/stderr restoration even on test failure). Also update error handling in pkg/config/load.go to use errors.As() consistently per errorlint requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
pkg/yaml/position_test.go (1)
11-235: Consider refactoring into table-driven tests.Many of these tests follow an identical pattern: parse YAML, extract positions, assert on specific paths. Consolidating into a table-driven test would reduce duplication and improve maintainability.
Example structure
func TestExtractPositions_Scenarios(t *testing.T) { tests := []struct { name string yaml string enabled bool wantPaths []string }{ { name: "disabled returns empty", yaml: `key: value`, enabled: false, wantPaths: []string{}, }, { name: "scalar value", yaml: `key: value`, enabled: true, wantPaths: []string{"key"}, }, { name: "nested mapping", yaml: ` parent: child1: value1 child2: value2`, enabled: true, wantPaths: []string{"parent", "parent.child1", "parent.child2"}, }, // ... additional cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var node goyaml.Node err := goyaml.Unmarshal([]byte(tt.yaml), &node) require.NoError(t, err) positions := ExtractPositions(&node, tt.enabled) for _, path := range tt.wantPaths { assert.True(t, HasPosition(positions, path), "missing path: %s", path) } }) } }Based on coding guidelines, table-driven tests are preferred for testing multiple scenarios.
pkg/function/git_root.go (4)
3-10: Add blank line between import groups.Per coding guidelines, separate stdlib and Atmos package imports with a blank line.
🔎 Proposed fix
import ( "context" "os/exec" "strings" + log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" )
17-28: Add blank line after perf.Track defer.Per coding guidelines, public functions should have a blank line after the perf.Track defer statement.
🔎 Proposed fix
func NewGitRootFunction() *GitRootFunction { defer perf.Track(nil, "function.NewGitRootFunction")() + return &GitRootFunction{
36-37: Add blank line after perf.Track defer.Per coding guidelines, public functions should have a blank line after the perf.Track defer statement.
🔎 Proposed fix
func (f *GitRootFunction) Execute(ctx context.Context, args string, execCtx *ExecutionContext) (any, error) { defer perf.Track(nil, "function.GitRootFunction.Execute")() + log.Debug("Executing repo-root function")
36-36: Consider documenting the args parameter behavior.The
argsparameter is not used in this function. Since!repo-rootdoesn't accept arguments, consider adding a comment explaining this or validating that args is empty to catch misuse early.internal/exec/terraform_test.go (1)
305-337: Consider aligning with concurrent pipe read pattern.Unlike other tests in this file,
TestExecuteTerraform_TerraformWorkspacereads from the pipe synchronously afterExecuteTerraformcompletes (line 327). If the workspace command produces output exceeding the pipe buffer (~64KB), this could deadlock.Other tests use a goroutine to read concurrently (see lines 88-94 for the pattern). Consider applying the same approach here for consistency and robustness.
🔎 Proposed refactor to add concurrent read
// Create a pipe to capture stdout to check if terraform is executed correctly. oldStdout := os.Stdout r, w, _ := os.Pipe() os.Stdout = w // Ensure stdout is restored even if test fails. defer func() { os.Stdout = oldStdout }() +// Read from pipe concurrently to avoid deadlock when output exceeds pipe buffer. +var buf bytes.Buffer +done := make(chan struct{}) +go func() { + _, _ = buf.ReadFrom(r) + close(done) +}() + err := ExecuteTerraform(info) // Close writer and restore stdout before checking error. w.Close() os.Stdout = oldStdout +// Wait for the reader goroutine to finish. +<-done + if err != nil { t.Fatalf("Failed to execute 'ExecuteTerraform': %v", err) } -// Read the captured output. -var buf bytes.Buffer -_, err = buf.ReadFrom(r) -if err != nil { - t.Fatalf("Failed to read from pipe: %v", err) -} output := buf.String()pkg/function/resolution/context.go (2)
173-198: Error message contains duplicate sentinel text.Line 177 writes the sentinel error to the builder, then line 197 wraps it again with
%w. When the error is printed, the sentinel message appears twice.Consider removing line 177 or restructuring to avoid redundancy.
🔎 Proposed fix
func (ctx *Context) buildCircularDependencyError(newNode DependencyNode) error { var builder strings.Builder - builder.WriteString(fmt.Sprintf("%s\n\n", errUtils.ErrCircularDependency)) - builder.WriteString("Dependency chain:\n") + builder.WriteString("Dependency chain:\n") // Show the full call stack. for i, node := range ctx.CallStack {
84-97: Consider usingLoadOrStorefor cleaner atomicity.The current Load-then-Store pattern is technically safe since goroutine IDs are unique, but
LoadOrStoreis more idiomatic for this pattern.🔎 Proposed alternative
func GetOrCreate() *Context { defer perf.Track(nil, "resolution.GetOrCreate")() gid := getGoroutineID() - if ctx, ok := goroutineContexts.Load(gid); ok { - return ctx.(*Context) - } - - ctx := NewContext() - goroutineContexts.Store(gid, ctx) - return ctx + ctx, _ := goroutineContexts.LoadOrStore(gid, NewContext()) + return ctx.(*Context) }One caveat: this always allocates a new context even when one exists. If allocation is a concern, the current approach is fine.
pkg/function/env.go (1)
3-12: Add blank line between import groups.Per coding guidelines, import groups should be separated by blank lines. Add a blank line between the stdlib imports (context, fmt, os, strings) and the Atmos package imports.
Proposed fix
import ( "context" "fmt" "os" "strings" + log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/utils" )Based on coding guidelines.
pkg/function/template.go (1)
3-10: Add blank line between import groups.Import groups should be separated by blank lines per coding guidelines.
Proposed fix
import ( "context" "encoding/json" "strings" + log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" )Based on coding guidelines.
pkg/function/store.go (1)
3-12: Add blank line between import groups.Import groups should be separated by blank lines per coding guidelines.
Proposed fix
import ( "context" "fmt" "strings" + log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/store" "github.com/cloudposse/atmos/pkg/utils" )Based on coding guidelines.
📜 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 (21)
internal/exec/packer_test.gointernal/exec/terraform_test.gointernal/exec/terraform_utils_test.gointernal/exec/workflow_adapters_test.gopkg/aws/identity/identity_test.gopkg/config/load.gopkg/function/aws.gopkg/function/aws_test.gopkg/function/context_test.gopkg/function/env.gopkg/function/git_root.gopkg/function/resolution/context.gopkg/function/resolution/context_test.gopkg/function/store.gopkg/function/store_test.gopkg/function/tags_test.gopkg/function/template.gopkg/function/template_test.gopkg/function/terraform_test.gopkg/yaml/position_test.gowebsite/blog/2025-12-18-function-registry-package.mdx
✅ Files skipped from review due to trivial changes (2)
- pkg/function/tags_test.go
- website/blog/2025-12-18-function-registry-package.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/function/aws.go
🧰 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: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in 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 for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/function/git_root.gointernal/exec/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/context_test.gopkg/function/terraform_test.gopkg/function/env.gopkg/function/store.gopkg/function/resolution/context.gopkg/yaml/position_test.gointernal/exec/packer_test.gopkg/function/template_test.gopkg/function/aws_test.gopkg/function/template.gointernal/exec/workflow_adapters_test.gointernal/exec/terraform_utils_test.gopkg/config/load.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: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
internal/exec/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/context_test.gopkg/function/terraform_test.gopkg/yaml/position_test.gointernal/exec/packer_test.gopkg/function/template_test.gopkg/function/aws_test.gointernal/exec/workflow_adapters_test.gointernal/exec/terraform_utils_test.go
🧠 Learnings (70)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Avoid utils package bloat; don't add new functions to pkg/utils/; create purpose-built packages instead (examples: pkg/store/, pkg/git/, pkg/pro/, pkg/filesystem/)
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: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level constants, variables, types, and functions declared in any file within a package are automatically accessible to all other files in the same package without imports or redeclaration. When reviewing Go code in the cloudposse/atmos repository (or any Go codebase), verify whether a seemingly undefined identifier exists in other files within the same package before suggesting to add it.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Organize imports in 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
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/function/git_root.gointernal/exec/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/context_test.gopkg/function/terraform_test.gopkg/function/env.gopkg/function/store.gopkg/function/resolution/context.gopkg/yaml/position_test.gointernal/exec/packer_test.gopkg/function/template_test.gopkg/function/aws_test.gopkg/function/template.gointernal/exec/workflow_adapters_test.gointernal/exec/terraform_utils_test.gopkg/config/load.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/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/function/context_test.gopkg/function/terraform_test.gointernal/exec/packer_test.gopkg/function/template_test.gointernal/exec/workflow_adapters_test.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/terraform_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
internal/exec/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/terraform_test.gointernal/exec/packer_test.gopkg/function/template_test.gopkg/function/aws_test.gointernal/exec/workflow_adapters_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
internal/exec/terraform_test.gopkg/function/context_test.gopkg/function/terraform_test.gointernal/exec/packer_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
internal/exec/terraform_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
internal/exec/terraform_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:
internal/exec/terraform_test.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/function/context_test.gopkg/function/terraform_test.gopkg/yaml/position_test.gopkg/function/template_test.gointernal/exec/workflow_adapters_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
internal/exec/terraform_test.gopkg/function/context_test.gopkg/function/env.gointernal/exec/workflow_adapters_test.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
internal/exec/terraform_test.gopkg/function/store.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/terraform_test.gopkg/function/terraform_test.gointernal/exec/terraform_utils_test.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/terraform_test.gointernal/exec/terraform_utils_test.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/terraform_test.gopkg/config/load.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/terraform_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
internal/exec/terraform_test.gopkg/config/load.go
📚 Learning: 2024-10-31T19:23:45.538Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:65-66
Timestamp: 2024-10-31T19:23:45.538Z
Learning: The variable `shouldCheckStack` in `ExecuteTerraform` controls whether validation is performed.
Applied to files:
internal/exec/terraform_test.gointernal/exec/terraform_utils_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/terraform_test.gopkg/function/terraform_test.gopkg/function/store.gopkg/function/template_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : UI (prompts, status) outputs go to stderr; data outputs go to stdout; logging is for system events only; never use logging for UI
Applied to files:
internal/exec/terraform_test.gointernal/exec/workflow_adapters_test.go
📚 Learning: 2024-11-10T19:28:17.365Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:0-0
Timestamp: 2024-11-10T19:28:17.365Z
Learning: When TTY is not supported, log the downgrade message at the Warn level using `u.LogWarning(cliConfig, ...)` instead of `fmt.Println`.
Applied to files:
internal/exec/terraform_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:
pkg/function/resolution/context_test.gopkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/terraform_test.gopkg/yaml/position_test.gopkg/function/template_test.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:
pkg/function/resolution/context_test.gopkg/function/resolution/context.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Use context.Context ONLY for cancellation signals, deadlines/timeouts, and request-scoped values (trace IDs, request IDs); context MUST be the first parameter in functions that accept it. DO NOT use context for passing configuration, dependencies, or avoiding proper function parameters
Applied to files:
pkg/function/resolution/context_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/aws_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
pkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/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 **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/function/store_test.gopkg/aws/identity/identity_test.gopkg/function/aws_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/function/context_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:
pkg/function/context_test.gopkg/function/terraform_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:
pkg/function/context_test.gopkg/function/aws_test.gointernal/exec/terraform_utils_test.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:
pkg/function/context_test.gopkg/config/load.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:
pkg/function/context_test.gointernal/exec/terraform_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/function/context_test.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
pkg/function/context_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/function/context_test.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:
pkg/function/terraform_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:
pkg/function/terraform_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:
pkg/function/terraform_test.gopkg/function/store.gopkg/function/template_test.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/function/terraform_test.gopkg/config/load.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/function/terraform_test.gopkg/function/store.gopkg/function/template_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
pkg/function/env.gopkg/function/store.go
📚 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:
pkg/function/env.go
📚 Learning: 2025-02-04T22:45:15.845Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-02-04T22:45:15.845Z
Learning: When validating URLs in Go, use the standard `url.Parse` function instead of character-based validation to properly handle URL-safe characters and query parameters.
Applied to files:
pkg/function/store.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:
pkg/function/resolution/context.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:
pkg/function/resolution/context.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:
pkg/function/resolution/context.go
📚 Learning: 2025-10-16T05:26:31.892Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1639
File: pkg/merge/merge.go:392-430
Timestamp: 2025-10-16T05:26:31.892Z
Learning: In Atmos merge operations (pkg/merge/merge.go), context.Positions is a PositionMap (map[string]*schema.Position) keyed by JSONPath strings, not a slice indexed by input array position. Position lookups are path-based, making them independent of input array reorganization or filtering. This design allows filtering empty inputs without affecting provenance tracking correctness.
Applied to files:
pkg/yaml/position_test.go
📚 Learning: 2025-04-24T01:06:15.259Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:06:15.259Z
Learning: When processing YAML structures with gopkg.in/yaml.v3, the Value field of a Node is only meant for scalar values. Complex data types like maps and arrays are represented through child nodes in the YAML tree structure.
Applied to files:
pkg/yaml/position_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
pkg/function/template_test.gopkg/function/template.go
📚 Learning: 2025-01-25T03:49:03.951Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/template_utils.go:268-271
Timestamp: 2025-01-25T03:49:03.951Z
Learning: The `ProcessTmplWithDatasourcesGomplate` function in `internal/exec/template_utils.go` is used for documentation generation purposes, where simple environment variable handling is acceptable and thread-safety concerns are not critical.
Applied to files:
pkg/function/template_test.gopkg/function/template.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:
pkg/function/template_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/workflow_adapters_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 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/workflow_adapters_test.gointernal/exec/terraform_utils_test.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/terraform_utils_test.go
📚 Learning: 2024-10-20T00:41:57.135Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 731
File: internal/exec/validate_stacks.go:93-98
Timestamp: 2024-10-20T00:41:57.135Z
Learning: When downloading schema files in `internal/exec/validate_stacks.go`, use a consistent temporary file name to overwrite the file each time and avoid creating multiple temporary files.
Applied to files:
internal/exec/terraform_utils_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/terraform_utils_test.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
internal/exec/terraform_utils_test.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
pkg/config/load.go
📚 Learning: 2024-10-20T13:06:20.839Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/utils/file_utils.go:177-194
Timestamp: 2024-10-20T13:06:20.839Z
Learning: In `pkg/utils/file_utils.go`, the `SearchConfigFile` function should focus on returning the file path and an `exists` boolean without returning an error.
Applied to files:
pkg/config/load.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:
pkg/config/load.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 : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
pkg/config/load.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 : Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Applied to files:
pkg/config/load.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
pkg/config/load.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:
pkg/config/load.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:
pkg/config/load.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
pkg/config/load.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use ATMOS_ prefix for environment variables with viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")
Applied to files:
pkg/config/load.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/config/load.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
pkg/config/load.go
📚 Learning: 2025-09-23T04:43:31.857Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1430
File: pkg/config/config.go:98-107
Timestamp: 2025-09-23T04:43:31.857Z
Learning: In the Atmos codebase, NO_PAGER environment variable handling is intentionally kept as direct os.Getenv() access in pkg/config/config.go rather than using Viper binding, because adding no_pager to the config file would be confusing for users. This is an acknowledged exception to the normal Viper binding pattern for environment variables.
Applied to files:
pkg/config/load.go
🧬 Code graph analysis (8)
internal/exec/terraform_test.go (1)
internal/exec/terraform.go (1)
ExecuteTerraform(45-675)
pkg/function/resolution/context_test.go (2)
pkg/perf/perf.go (1)
CallStack(53-56)pkg/function/resolution/context.go (3)
Clear(100-105)GetOrCreate(85-97)Scoped(113-136)
pkg/yaml/position_test.go (1)
pkg/yaml/position.go (3)
ExtractPositions(21-31)HasPosition(125-134)GetPosition(109-122)
pkg/function/template_test.go (3)
pkg/function/template.go (1)
NewTemplateFunction(18-28)pkg/function/tags.go (2)
TagTemplate(21-21)YAMLTag(108-112)pkg/function/phase.go (2)
PreMerge(13-13)Phase(8-8)
pkg/function/aws_test.go (3)
pkg/aws/identity/identity.go (4)
CallerIdentity(21-26)GetCallerIdentity(178-216)ClearIdentityCache(166-172)SetGetter(84-92)pkg/schema/schema.go (3)
AtmosConfiguration(54-99)ConfigAndStacksInfo(667-763)AuthContext(615-625)pkg/function/aws.go (4)
NewAwsAccountIDFunction(42-52)NewAwsCallerIdentityArnFunction(79-89)NewAwsCallerIdentityUserIDFunction(116-126)NewAwsRegionFunction(153-163)
internal/exec/workflow_adapters_test.go (1)
pkg/terminal/terminal.go (1)
Stderr(85-85)
internal/exec/terraform_utils_test.go (1)
tests/preconditions.go (1)
RequireExecutable(322-334)
pkg/config/load.go (1)
pkg/logger/log.go (1)
Debug(24-26)
⏰ 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
|
@coderabbitai full review please |
|
@aknysh: I'll perform a full review of the PR changes now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
pkg/function/include_test.go (1)
11-31: Consider using table-driven tests to reduce duplication.The Execute tests for Include and IncludeRaw follow nearly identical patterns. Per coding guidelines, table-driven tests are preferred for testing multiple scenarios.
🔎 Proposed table-driven test refactor
-func TestIncludeFunction_Execute(t *testing.T) { - fn := NewIncludeFunction() - - // Include function always returns ErrSpecialYAMLHandling. - result, err := fn.Execute(context.Background(), "path/to/file.yaml", nil) - require.Error(t, err) - assert.Nil(t, result) - assert.ErrorIs(t, err, ErrSpecialYAMLHandling) - assert.Contains(t, err.Error(), "include") -} - -func TestIncludeRawFunction_Execute(t *testing.T) { - fn := NewIncludeRawFunction() - - // Include.raw function always returns ErrSpecialYAMLHandling. - result, err := fn.Execute(context.Background(), "path/to/file.txt", nil) - require.Error(t, err) - assert.Nil(t, result) - assert.ErrorIs(t, err, ErrSpecialYAMLHandling) - assert.Contains(t, err.Error(), "include.raw") -} +func TestIncludeFunction_Execute(t *testing.T) { + tests := []struct { + name string + fn Function + input string + expectedSubstr string + }{ + { + name: "include", + fn: NewIncludeFunction(), + input: "path/to/file.yaml", + expectedSubstr: "include", + }, + { + name: "include.raw", + fn: NewIncludeRawFunction(), + input: "path/to/file.txt", + expectedSubstr: "include.raw", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := tt.fn.Execute(context.Background(), tt.input, nil) + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrSpecialYAMLHandling) + assert.Contains(t, err.Error(), tt.expectedSubstr) + }) + } +}pkg/function/phase_test.go (1)
40-44: Consider whether testing exact constant values adds value.This test verifies the specific numeric values of iota constants. Unless these values are part of a stable API contract (e.g., for serialization or protocol compatibility), testing that
PreMerge == Phase(0)is tautological—it just confirms what the code defines.If ordering matters more than exact values, you could test
PreMerge < PostMergeinstead. Otherwise, this test could be removed.pkg/yaml/types_test.go (2)
70-73: Remove tautological constant test.Testing that a constant equals its declared value doesn't add meaningful coverage. Constants can't change at runtime, so this test only verifies Go's const mechanism.
Based on coding guidelines that state "avoid tautological tests."
75-85: Remove tautological struct field test.Testing basic struct field assignment (setting
Indent: 4and verifying it equals4) doesn't validate behavior. This would only be valuable ifOptionshad validation logic, computed properties, or methods to test.Based on coding guidelines that state "avoid tautological tests."
pkg/function/git_root_test.go (2)
13-33: Consider adding conditional skip for non-git environments.This integration test assumes execution within a git repository, making it fragile and environment-dependent. If the test runs outside a git context (e.g., in certain CI environments or when testing the package in isolation), it will fail.
Consider adding
t.Skip()when not in a git repo, or refactor to use dependency injection with a mockable git interface for true unit testing.🔎 Example with conditional skip
func TestGitRootFunction_Execute_InGitRepo(t *testing.T) { + // Skip if not in a git repository + if _, err := os.Stat(".git"); os.IsNotExist(err) { + t.Skip("Not in a git repository, skipping integration test") + } + fn := NewGitRootFunction() // This test assumes we're running in a git repository.As per coding guidelines: prefer unit tests with mocks over integration tests.
35-48: Consider consolidating metadata tests or using table-driven approach.
TestGitRootFunction_Aliasesduplicates alias validation already covered inTestNewGitRootFunction(line 40). You could consolidate these into a single metadata test or use a table-driven structure for checking Name/Phase/Aliases properties.🔎 Example consolidation
func TestNewGitRootFunction(t *testing.T) { fn := NewGitRootFunction() require.NotNil(t, fn) assert.Equal(t, TagRepoRoot, fn.Name()) assert.Equal(t, PreMerge, fn.Phase()) - assert.Contains(t, fn.Aliases(), "git-root") -} - -func TestGitRootFunction_Aliases(t *testing.T) { - fn := NewGitRootFunction() - aliases := fn.Aliases() + + // Validate aliases + aliases := fn.Aliases() assert.Len(t, aliases, 1) assert.Equal(t, "git-root", aliases[0]) }pkg/function/env_test.go (4)
73-80: Consider using sentinel errors instead of string matching.Line 79 checks error message content with
assert.Contains, which doesn't follow the guideline to useerrors.Is()for error checking. While validation errors often use string matching, wrapping argument validation errors in a defined sentinel (e.g.,ErrInvalidArgCount) would be more robust and testable.As per coding guidelines: all errors should be wrapped using static errors from errors/errors.go.
82-127: Consider whether internal helper warrants direct testing.
parseEnvArgsis unexported, and testing internal helpers directly can couple tests to implementation details. If the parsing logic is complex enough to justify independent testing, this is fine. Otherwise, consider testing this behavior exclusively through the publicExecutemethod.The test structure itself is solid—just verify that direct testing of this helper aligns with your testing strategy.
129-140: Minimal coverage for internal helper.Similar to
parseEnvArgs, this tests an unexported helper directly. Additionally, this only covers nil/empty cases without positive test cases. Consider whether this helper's behavior is sufficiently tested through the public API or if positive cases are missing here.
142-148: Constructor test may be tautological.This test verifies the constructor returns specific constant values (
TagEnv,PreMerge,nilaliases). If these properties are part of the Function interface contract and consumers depend on them, this is valid. Otherwise, it's testing that the constructor does exactly what it says it does, which can be considered a tautology.Consider whether these properties are sufficiently validated through integration tests or if this constructor-level check adds value.
pkg/function/store_test.go (1)
590-622: Consider removing tautological struct tests.These tests verify basic struct field assignment, which is Go language behavior rather than business logic. If struct assignment were broken, the functional tests above would already fail. Removing these tests reduces maintenance overhead without losing meaningful coverage.
Based on coding guidelines: "Test behavior, not implementation; avoid tautological tests".
pkg/function/exec_test.go (2)
20-36: Refactor to table-driven test.These two test functions validate the same error condition with different inputs. Consolidate them into a single table-driven test per Go best practices.
🔎 Proposed refactor using table-driven test
-func TestExecFunction_Execute_EmptyArgs(t *testing.T) { - fn := NewExecFunction() - - // Empty args should return error. - _, err := fn.Execute(context.Background(), "", nil) - require.Error(t, err) - assert.ErrorIs(t, err, ErrInvalidArguments) -} - -func TestExecFunction_Execute_WhitespaceOnly(t *testing.T) { - fn := NewExecFunction() - - // Whitespace-only args should return error. - _, err := fn.Execute(context.Background(), " ", nil) - require.Error(t, err) - assert.ErrorIs(t, err, ErrInvalidArguments) -} +func TestExecFunction_Execute_InvalidArgs(t *testing.T) { + tests := []struct { + name string + args string + }{ + {"empty args", ""}, + {"whitespace only", " "}, + } + + fn := NewExecFunction() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := fn.Execute(context.Background(), tt.args, nil) + require.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidArguments) + }) + } +}Based on coding guidelines.
38-71: Consider table-driven test for output parsing scenarios.The three output parsing tests share similar structure and could be consolidated into a table-driven test to reduce duplication and improve maintainability.
🔎 Proposed refactor using table-driven test
-func TestExecFunction_Execute_JSONOutput(t *testing.T) { - fn := NewExecFunction() - - // Test command that outputs JSON. - result, err := fn.Execute(context.Background(), `echo '{"key": "value"}'`, nil) - require.NoError(t, err) - - // Result should be parsed as map. - m, ok := result.(map[string]any) - require.True(t, ok) - assert.Equal(t, "value", m["key"]) -} - -func TestExecFunction_Execute_JSONArray(t *testing.T) { - fn := NewExecFunction() - - // Test command that outputs JSON array. - result, err := fn.Execute(context.Background(), `echo '[1, 2, 3]'`, nil) - require.NoError(t, err) - - // Result should be parsed as slice. - arr, ok := result.([]any) - require.True(t, ok) - assert.Len(t, arr, 3) -} - -func TestExecFunction_Execute_NonJSONOutput(t *testing.T) { - fn := NewExecFunction() - - // Test command that outputs non-JSON (echo adds a newline). - result, err := fn.Execute(context.Background(), "echo 'not json'", nil) - require.NoError(t, err) - assert.Equal(t, "not json\n", result) -} +func TestExecFunction_Execute_OutputParsing(t *testing.T) { + tests := []struct { + name string + cmd string + validator func(t *testing.T, result any) + }{ + { + name: "JSON object", + cmd: `echo '{"key": "value"}'`, + validator: func(t *testing.T, result any) { + m, ok := result.(map[string]any) + require.True(t, ok) + assert.Equal(t, "value", m["key"]) + }, + }, + { + name: "JSON array", + cmd: `echo '[1, 2, 3]'`, + validator: func(t *testing.T, result any) { + arr, ok := result.([]any) + require.True(t, ok) + assert.Len(t, arr, 3) + }, + }, + { + name: "non-JSON output", + cmd: "echo 'not json'", + validator: func(t *testing.T, result any) { + assert.Equal(t, "not json\n", result) + }, + }, + } + + fn := NewExecFunction() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := fn.Execute(context.Background(), tt.cmd, nil) + require.NoError(t, err) + tt.validator(t, result) + }) + } +}Based on coding guidelines.
📜 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 (12)
pkg/function/env_test.gopkg/function/exec_test.gopkg/function/git_root.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/phase_test.gopkg/function/random_test.gopkg/function/registry_test.gopkg/function/resolution/context.gopkg/function/resolution/context_test.gopkg/function/store_test.gopkg/yaml/types_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/function/git_root.go
- pkg/function/resolution/context_test.go
🧰 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: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in 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 for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/function/resolution/context.gopkg/yaml/types_test.gopkg/function/store_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: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_test.go
🧠 Learnings (27)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
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: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Avoid utils package bloat; don't add new functions to pkg/utils/; create purpose-built packages instead (examples: pkg/store/, pkg/git/, pkg/pro/, pkg/filesystem/)
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: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
📚 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:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_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:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_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/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/include_test.gopkg/yaml/types_test.gopkg/function/store_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/env_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/function/phase_test.gopkg/function/git_root_test.gopkg/function/registry_test.gopkg/function/random_test.gopkg/function/exec_test.gopkg/function/env_test.gopkg/function/include_test.gopkg/function/resolution/context.gopkg/yaml/types_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to cmd/**/*_test.go : ALWAYS use cmd.NewTestKit(t) for cmd tests to auto-clean RootCmd state (flags, args)
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/function/registry_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
pkg/function/registry_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limit
Applied to files:
pkg/function/exec_test.gopkg/yaml/types_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:
pkg/function/env_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:
pkg/function/env_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/function/env_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:
pkg/function/env_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
pkg/function/resolution/context.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Use context.Context ONLY for cancellation signals, deadlines/timeouts, and request-scoped values (trace IDs, request IDs); context MUST be the first parameter in functions that accept it. DO NOT use context for passing configuration, dependencies, or avoiding proper function parameters
Applied to files:
pkg/function/resolution/context.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:
pkg/function/resolution/context.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:
pkg/function/resolution/context.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:
pkg/function/resolution/context.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:
pkg/function/resolution/context.go
📚 Learning: 2025-04-24T01:06:15.259Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:06:15.259Z
Learning: When processing YAML structures with gopkg.in/yaml.v3, the Value field of a Node is only meant for scalar values. Complex data types like maps and arrays are represented through child nodes in the YAML tree structure.
Applied to files:
pkg/yaml/types_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:
pkg/function/store_test.go
🧬 Code graph analysis (7)
pkg/function/phase_test.go (1)
pkg/function/phase.go (3)
Phase(8-8)PreMerge(13-13)PostMerge(17-17)
pkg/function/git_root_test.go (4)
pkg/function/git_root.go (1)
NewGitRootFunction(20-30)pkg/function/tags.go (1)
TagRepoRoot(39-39)pkg/function/phase.go (2)
PreMerge(13-13)Phase(8-8)internal/tui/templates/base_template.go (1)
Aliases(8-8)
pkg/function/random_test.go (3)
pkg/function/random.go (1)
NewRandomFunction(28-38)pkg/function/tags.go (1)
TagRandom(42-42)pkg/function/phase.go (2)
PreMerge(13-13)Phase(8-8)
pkg/function/env_test.go (3)
pkg/function/env.go (1)
NewEnvFunction(20-30)pkg/function/tags.go (1)
TagEnv(30-30)pkg/function/phase.go (2)
PreMerge(13-13)Phase(8-8)
pkg/function/resolution/context.go (4)
pkg/perf/perf.go (1)
Track(121-138)pkg/schema/schema.go (1)
AtmosConfiguration(54-99)pkg/function/errors.go (1)
ErrCircularDependency(20-20)pkg/function/resolution/errors.go (1)
ErrCircularDependency(6-6)
pkg/yaml/types_test.go (1)
pkg/yaml/types.go (1)
DefaultIndent(9-9)
pkg/function/store_test.go (5)
pkg/function/store.go (2)
NewStoreFunction(20-30)NewStoreGetFunction(86-96)pkg/function/tags.go (2)
TagStore(15-15)TagStoreGet(18-18)pkg/function/phase.go (2)
PostMerge(17-17)Phase(8-8)pkg/store/store.go (1)
Store(6-10)pkg/function/errors.go (1)
ErrExecutionFailed(17-17)
⏰ 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 (16)
pkg/function/include_test.go (1)
33-47: Constructor tests look good.Tests appropriately validate the Function interface contract (Name, Phase, Aliases), which is behavioral in the context of the registry pattern. The metadata validated here is used for function lookup and orchestration.
pkg/function/phase_test.go (1)
9-38: Solid table-driven test for Phase.String().Good coverage of all Phase.String() cases including the edge case for unknown phases. Clean and straightforward.
pkg/yaml/types_test.go (1)
11-51: Solid table-driven test coverage.The test properly validates marshaling behavior across multiple scenarios with appropriate assertions for node kind, style, and value.
pkg/function/git_root_test.go (1)
3-11: Add blank line between import groups.Per coding guidelines, imports should be organized in groups separated by blank lines: stdlib, 3rd-party, atmos packages.
🔎 Proposed fix
import ( "context" "os" "path/filepath" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" )⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checkingLearnt from: samtholiya Repo: cloudposse/atmos PR: 1255 File: cmd/describe_affected_test.go:15-15 Timestamp: 2025-05-23T19:51:47.091Z Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.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 fixturesLearnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to cmd/**/*_test.go : ALWAYS use cmd.NewTestKit(t) for cmd tests to auto-clean RootCmd state (flags, args)Learnt from: osterman Repo: cloudposse/atmos PR: 1808 File: cmd/terraform/backend/backend_delete_test.go:9-23 Timestamp: 2025-12-10T18:32:51.237Z Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.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 packagesLearnt 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 GoLearnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Organize imports in 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, errUtilsLearnt 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 implementationsLearnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T18:20:55.630Z Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limitLearnt from: osterman Repo: cloudposse/atmos PR: 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.pkg/function/env_test.go (1)
11-71: Comprehensive edge case coverage with proper test hygiene.The table-driven structure and use of
t.Setenvalign with best practices. The test cases cover meaningful scenarios including empty/whitespace args, existing/missing vars, and default value handling.One note: Line 62 passes
nilas theExecutionContextparameter. Verify this is the intended behavior for testing edge cases rather than testing with a minimal valid context.pkg/function/store_test.go (1)
43-588: Excellent test coverage and structure.The test suite is comprehensive and well-organized:
- Table-driven tests throughout
- Covers happy paths, error conditions, and edge cases
- Proper use of
errors.Is()for error checking- Tests behavior rather than implementation details
This aligns perfectly with the project's testing standards.
pkg/function/registry_test.go (1)
1-431: Excellent test coverage and structure.This test file provides comprehensive coverage of the function registry with well-structured tests covering registration, retrieval, phase-based filtering, alias handling, case-insensitivity, conflict detection, and function execution integration. The tests follow best practices with proper use of
errors.Is(), testify assertions, and behavior-focused validation. Edge cases like duplicate registration, non-existent lookups, and empty registry operations are all covered.pkg/function/resolution/context.go (5)
1-40: Clean type definitions and proper import organization.The package structure follows guidelines with properly organized imports (stdlib, then atmos packages) and well-documented types. The
DependencyNodeandContextstructures are clearly defined for tracking dependency resolution chains.
42-82: Goroutine ID parsing is robust with proper fallback handling.The implementation correctly addresses the previous review feedback by using an atomic counter to generate unique fallback IDs (
unknown-N) when parsing fails, preventing metric collisions. The dynamic buffer growth with a safety limit is a solid defensive approach.
84-136: Context lifecycle management is well-designed.The
GetOrCreate,Clear, andScopedfunctions provide proper per-goroutine context management with appropriate performance tracking. TheScopedpattern with a restore function is excellent for preventing memory leaks and cross-call contamination.
138-171: Cycle detection logic is sound and efficient.The
PushandPopmethods correctly maintain the call stack and visited set for circular dependency detection. The implementation properly checks for cycles before adding nodes and maintains consistency when popping.
173-218: Error reporting and cloning are well-implemented.The
buildCircularDependencyErrorfunction creates detailed, actionable error messages showing the complete dependency chain and helpful resolution steps. TheClonemethod properly performs a deep copy of both the call stack and visited map, with appropriate nil handling and capacity pre-allocation for efficiency.pkg/function/random_test.go (3)
71-83: Good boundary testing.The boundary value test with min=0, max=1 effectively validates range constraints across multiple iterations. Well structured.
85-95: LGTM!Negative value handling is tested appropriately with clear range validation.
97-103: LGTM!Constructor test validates the public interface contract correctly.
pkg/function/exec_test.go (1)
11-71: Cross-platform compatibility is already handled—no action needed.The implementation uses mvdan.cc/sh/v3, a pure Go POSIX shell parser and interpreter that provides consistent shell semantics across all platforms. Single quotes in test commands like
echo '{"key": "value"}'are interpreted identically on Windows, Linux, and macOS. Tests will work correctly on all platforms without modification.
|
@aknysh, I'll perform a comprehensive review of all the changes in this PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/function/resolution/context_test.go (1)
359-395: Concurrent access test fix verified.Previous review correctly identified the issue with
require.NotNilinside goroutines. Now usingassert.NotNil(line 374) and properly capturing the Push error withassert.NoError(line 385). Good fix.
🧹 Nitpick comments (15)
internal/exec/terraform_utils_test.go (2)
113-116: Consider consolidating cleanup in defer.The defer at line 114-116 and explicit restoration at line 144-145 both restore
os.Stderr, creating redundancy. While this defensive approach ensures restoration in all cases, a more idiomatic pattern would consolidate bothw.Close()andos.Stderrrestoration in the defer:- // Ensure stderr is restored even if test fails. - defer func() { + defer func() { + w.Close() os.Stderr = oldStd }() stack := "prod" ... err = ExecuteTerraformAffected(&a, &info) - - // Restore stderr before checking error. - w.Close() - os.Stderr = oldStd if err != nil {This eliminates the double restoration while maintaining cleanup safety.
Also applies to: 141-145
167-170: Consider consolidating cleanup in defer.Same pattern as
TestExecuteTerraformAffectedWithDependents: the defer and explicit restoration create redundancy. Consolidating cleanup operations in the defer would be cleaner:- // Ensure stderr is restored even if test fails. - defer func() { + defer func() { + w.Close() os.Stderr = oldStd }() stack := "prod" ... err := ExecuteTerraformQuery(&info) - - // Restore stderr before checking error. - w.Close() - os.Stderr = oldStd if err != nil {Also applies to: 183-186
internal/exec/packer_test.go (1)
13-33: Consider adding edge case test for non-git directory.The test assumes execution in a git repository. Consider adding a test case for Execute behavior when called from a non-git directory to ensure graceful error handling.
Optional test case
func TestGitRootFunction_Execute_NotInGitRepo(t *testing.T) { fn := NewGitRootFunction() // Create a temp directory outside any git repo tmpDir := t.TempDir() originalWd, _ := os.Getwd() defer os.Chdir(originalWd) os.Chdir(tmpDir) _, err := fn.Execute(context.Background(), "", nil) assert.Error(t, err) }pkg/function/phase.go (1)
20-32: Consider removing perf tracking from this trivial accessor.The
String()method is a simple switch statement. Based on learnings, trivial accessors like this don't needperf.Track()since the tracking overhead would exceed the actual method cost. Consider removing thedefer perf.Track(nil, "function.Phase.String")()line.🔎 Proposed change
// String returns a human-readable representation of the phase. func (p Phase) String() string { - defer perf.Track(nil, "function.Phase.String")() - switch p { case PreMerge: return "pre-merge" case PostMerge: return "post-merge" default: return "unknown" } }Based on learnings, hot-path methods with only trivial operations should avoid perf.Track().
pkg/aws/identity/identity_test.go (2)
14-28: Consider generating mock with mockgen.Manual mock works here, but coding guidelines prefer
go.uber.org/mock/mockgenfor consistency. A//go:generatedirective inidentity.gowould enable this.
160-172: TestCallerIdentity is somewhat tautological.This test only verifies that struct fields hold assigned values. Consider removing or replacing with a test that validates actual behavior (e.g., field extraction from API response).
pkg/function/exec.go (2)
49-52: Wrap shell execution error with context.Per error handling guidelines, wrap the error with meaningful context to aid debugging.
🔎 Proposed fix
res, err := utils.ExecuteShellAndReturnOutput(args, YAMLTag(TagExec)+" "+args, ".", os.Environ(), false) if err != nil { - return nil, err + return nil, fmt.Errorf("exec function failed for command %q: %w", args, err) }You'll need to add
"fmt"to imports.
49-49: Consider using BaseDir from ExecutionContext.The working directory is hardcoded to
".". IfexecCtxis non-nil andexecCtx.BaseDiris set, it might be more appropriate to execute from that directory for relative path resolution consistency.pkg/function/template_test.go (1)
44-65: Consider consolidating JSON type tests into table-driven format.Tests for JSONNumber, JSONBoolean could be combined into a single table-driven test. Not critical, just a minor cleanup opportunity.
pkg/function/aws_test.go (1)
15-29: Consider using generated mock for consistency.Same suggestion as
identity_test.go- a generated mock viamockgenwould align with coding guidelines.pkg/function/terraform_test.go (1)
234-244: Consider removing tautological struct test.This test only verifies that Go struct assignment works correctly—it doesn't test any behavior of your code. Tests should validate behavior, not language mechanics. As per coding guidelines, avoid tautological tests.
internal/exec/terraform_test.go (1)
718-724: No-op logger reset.Line 721
log.SetDefault(log.Default())sets the default logger to itself—this has no effect. Looks like leftover from refactoring. Consider removing this line.🔎 Proposed fix
// Restore stdout, stderr, and logger. os.Stdout = oldStdout os.Stderr = oldStderr - log.SetDefault(log.Default())pkg/function/tags.go (2)
64-85: Consider using a package-level constant slice instead of a function.
AllTags()allocates a new slice on every call. Since the tag list is fixed at compile time, consider replacing it with a package-level variable:var allTags = []string{ TagExec, TagStore, // ... rest }This eliminates repeated allocations while maintaining the same API.
🔎 Proposed refactor
-// AllTags returns all registered tag names. -func AllTags() []string { - defer perf.Track(nil, "function.AllTags")() - - return []string{ - TagExec, - TagStore, - TagStoreGet, - TagTemplate, - TagTerraformOutput, - TagTerraformState, - TagEnv, - TagInclude, - TagIncludeRaw, - TagRepoRoot, - TagRandom, - TagLiteral, - TagAwsAccountID, - TagAwsCallerIdentityArn, - TagAwsCallerIdentityUserID, - TagAwsRegion, - } -} +// allTags holds all registered tag names. +var allTags = []string{ + TagExec, + TagStore, + TagStoreGet, + TagTemplate, + TagTerraformOutput, + TagTerraformState, + TagEnv, + TagInclude, + TagIncludeRaw, + TagRepoRoot, + TagRandom, + TagLiteral, + TagAwsAccountID, + TagAwsCallerIdentityArn, + TagAwsCallerIdentityUserID, + TagAwsRegion, +} + +// AllTags returns all registered tag names. +func AllTags() []string { + defer perf.Track(nil, "function.AllTags")() + return allTags +}
108-112: Performance tracking overhead may exceed function cost.
YAMLTagandFromYAMLTagperform trivial string operations (concatenation and slice), yet include perf tracking. The tracking overhead likely exceeds the actual work. Per learnings, perf.Track should be skipped for hot-path trivial operations.🔎 Proposed refactor
// YAMLTag returns the YAML tag format for a function name (e.g., "env" -> "!env"). func YAMLTag(name string) string { - defer perf.Track(nil, "function.YAMLTag")() - return YAMLTagPrefix + name } // FromYAMLTag extracts the function name from a YAML tag (e.g., "!env" -> "env"). func FromYAMLTag(tag string) string { - defer perf.Track(nil, "function.FromYAMLTag")() - if len(tag) > 0 && tag[0] == '!' { return tag[1:] } return tag }Based on learnings, hot-path methods with trivial operations should avoid perf.Track() when tracking overhead exceeds actual work.
Also applies to: 115-122
pkg/function/git_root_test.go (1)
43-48: Minor duplication with constructor test.This test overlaps with line 40 of TestNewGitRootFunction which already checks aliases. The duplication provides focused coverage for aliases but could be consolidated into table-driven tests if you expand test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pkg/function/exec_test.go (1)
11-18: Security concern already raised in prior review.The command injection risk identified in the previous review comment remains valid. The ExecFunction passes commands directly to shell execution without validation. If this is exposed to user-provided configuration values, input validation or command allowlisting should be enforced.
pkg/function/random_test.go (2)
11-69: Good table-driven test structure for error cases.The test organization follows guidelines well with clear test case names and expectations.
However, the error assertions at line 63 should use
errors.Is(err, ErrInvalidArguments)instead of substring matching per coding guidelines.
105-118: Useerrors.Is()for sentinel error checking.Line 117 checks error message with
assert.Contains. SincegenerateRandomwrapsErrInvalidArguments, useerrors.Is(err, ErrInvalidArguments)for proper sentinel verification.pkg/yaml/types_test.go (1)
53-68: Strengthen the integration test assertion.The comment claims to verify folded style (
>), but line 67 only checks for"description:". Assert"description: >"to actually validate the folded scalar format.pkg/function/store_test.go (1)
15-41: Manual mock noted - follow-up per prior feedback.This was already flagged in a previous review. The current implementation works; transitioning to mockgen can happen in a follow-up.
🧹 Nitpick comments (12)
pkg/function/phase.go (1)
20-32: Removeperf.Trackfrom trivial String() method.String() performs only a 3-case switch returning literals. The perf tracking overhead exceeds the method's actual cost, especially since String() is likely called frequently for logging/debugging.
Based on learnings, defer perf.Track() should be excluded from "trivial accessors/mutators where the tracking overhead would exceed the actual method cost."
🔎 Proposed fix
func (p Phase) String() string { - defer perf.Track(nil, "function.Phase.String")() - switch p { case PreMerge: return "pre-merge"pkg/function/context.go (1)
41-66: Removeperf.Trackfrom trivial mutator methods.WithFile, WithBaseDir, and WithStackInfo are trivial mutators that only copy a struct and update a single field. The perf tracking overhead exceeds the actual work performed (struct copy + field assignment).
Based on learnings, defer perf.Track() should be excluded from "trivial accessors/mutators where the tracking overhead would exceed the actual method cost."
🔎 Proposed fix
func (ctx *ExecutionContext) WithFile(file string) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithFile")() - newCtx := *ctx newCtx.File = file return &newCtx } func (ctx *ExecutionContext) WithBaseDir(baseDir string) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithBaseDir")() - newCtx := *ctx newCtx.BaseDir = baseDir return &newCtx } func (ctx *ExecutionContext) WithStackInfo(stackInfo *schema.ConfigAndStacksInfo) *ExecutionContext { - defer perf.Track(nil, "function.ExecutionContext.WithStackInfo")() - newCtx := *ctx newCtx.StackInfo = stackInfo return &newCtx }pkg/function/function.go (1)
25-52: Consider perf.Track overhead on trivial getters.
Name(),Aliases(), andPhase()are simple field accessors. Theperf.Trackcalls add overhead for minimal benefit here. These methods may be called frequently during registry lookups.That said, per guidelines, public functions should have tracking—so this is compliant. Just flagging for awareness.
pkg/function/exec.go (1)
39-62: Consider wrapping the shell execution error with context.Line 51 returns
errdirectly fromExecuteShellAndReturnOutput. Per coding guidelines, errors should be wrapped with context usingfmt.Errorf("context: %w", err).🔎 Proposed fix
res, err := utils.ExecuteShellAndReturnOutput(args, YAMLTag(TagExec)+" "+args, ".", os.Environ(), false) if err != nil { - return nil, err + return nil, fmt.Errorf("exec function failed: %w", err) }You'll also need to add
"fmt"to the imports.internal/exec/terraform_test.go (1)
305-337: Minor inconsistency: synchronous pipe read.Unlike other tests in this file,
TestExecuteTerraform_TerraformWorkspacereads from the pipe synchronously (lines 326-331) rather than using a concurrent reader goroutine. This works for small outputs but could theoretically block if output grows. Consider aligning with the concurrent pattern used elsewhere for consistency.pkg/function/env.go (1)
39-42: Error wrapping loses context.When
SplitStringByDelimiterfails, the original error is discarded. Consider preserving it for debugging.🔎 Proposed fix
parts, err := utils.SplitStringByDelimiter(args, ' ') if err != nil { - return "", "", fmt.Errorf("%w: %s", ErrInvalidArguments, args) + return "", "", fmt.Errorf("%w: failed to parse args %q: %w", ErrInvalidArguments, args, err) }pkg/function/terraform.go (1)
102-153: Consider extracting shared validation logic.
TerraformStateFunction.Executeis nearly identical toTerraformOutputFunction.Execute. Once the actual implementations land, you might benefit from a shared helper for the common pre-validation and argument parsing. Not blocking - just something to keep in mind for the migration.pkg/function/tags.go (1)
63-105: AllTags and TagsMap provide flexibility for tag lookups.
AllTags()allocates on each call which is fine for infrequent use. Note thatTagsMapis technically mutable since it's an exportedvar—external code could modify it. If immutability is important, consider a getter function instead.pkg/function/store.go (1)
60-77: Consider handling nil values consistently with store.get.The
storefunction returns defaults only on retrieval errors (line 63), butstore.get(viaretrieveFromStoreat lines 108-111) also returns defaults when the retrieved value isnil. Should both functions handle nil values the same way?Proposed alignment with store.get behavior
// Retrieve the value from the store. value, err := store.Get(params.stack, params.component, params.key) if err != nil { if params.defaultValue != nil { return *params.defaultValue, nil } return nil, fmt.Errorf("%w: failed to get key '%s': %w", ErrExecutionFailed, params.key, err) } + + // Check if the retrieved value is nil and use default if provided. + if value == nil && params.defaultValue != nil { + return *params.defaultValue, nil + } // Execute the YQ expression if provided.pkg/aws/identity/identity.go (1)
301-314: Consider wrapping the error on config reload.Line 313 returns the result of
config.LoadDefaultConfigfor assumed role credentials but doesn't wrap the error with context. For consistency with line 297, should this error also be wrapped witherrUtils.ErrLoadAWSConfig?Proposed fix
// Reload full config with assumed role credentials. - return config.LoadDefaultConfig(ctx, cfgOpts...) + assumedCfg, err := config.LoadDefaultConfig(ctx, cfgOpts...) + if err != nil { + return aws.Config{}, fmt.Errorf("%w: %w", errUtils.ErrLoadAWSConfig, err) + } + return assumedCfg, nil } return baseCfg, nilpkg/function/resolution/context.go (2)
29-30: Consider adding a godoc comment for package-level variables.While
goroutineContextsis unexported, adding a brief comment would improve code readability and explain its purpose for future maintainers.
49-50: Consider adding a godoc comment for the counter variable.A brief comment explaining that
unknownIDCountergenerates unique IDs when goroutine ID parsing fails would improve code documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/function/exec.go (1)
40-63: Execute implementation is clean with understood security trade-offs.The function directly executes shell commands from stack manifests (line 50). The past review identified this as a command injection vector, which is correct. However, this appears to be an intentional design decision where stack manifests are treated as trusted infrastructure code. The use of
mvdan.cc/sh/v3provides some isolation compared to direct shell execution.
🧹 Nitpick comments (4)
pkg/function/git_root_test.go (1)
14-42: Add error case tests to improve coverage.The test only covers the happy path. Consider adding tests for error scenarios to reach >80% coverage:
- Execute behavior when git commands fail (mock or simulate)
- Execute behavior with different input arguments
- Edge cases like nested git repositories or worktrees
Additionally, this is more of an integration test than a unit test since it calls actual git commands. Per coding guidelines, consider refactoring the implementation to accept a git command executor interface that can be mocked, enabling true unit tests with dependency injection.
pkg/store/store.go (1)
6-12: Consider adding documentation for GetKey method.While the go:generate directive is correctly placed, the newly added GetKey method lacks a comment explaining its purpose. Adding a brief comment would improve API clarity.
Suggested documentation
//go:generate go run go.uber.org/mock/mockgen@v0.6.0 -source=$GOFILE -destination=mock_store.go -package=store type Store interface { Set(stack string, component string, key string, value any) error Get(stack string, component string, key string) (any, error) + // GetKey retrieves a value directly by key without stack or component context. GetKey(key string) (any, error) }pkg/function/phase.go (1)
17-26: Consider adding performance tracking for consistency.The repository policy enforces
defer perf.Track(nil, "function.Phase.String")()on all public functions, including simple getters. While the overhead might exceed this trivial operation's work, consistency with linting rules is valued.🔎 Optional enhancement
func (p Phase) String() string { + defer perf.Track(nil, "function.Phase.String")() + switch p { case PreMerge: return "pre-merge" case PostMerge: return "post-merge" default: return "unknown" } }pkg/function/context.go (1)
41-60: Immutable builder pattern is well-implemented.The With* methods correctly return copies, preventing mutation bugs and ensuring thread-safety.
Minor consistency note: per repository policy, public methods should include
defer perf.Track(...). While these are trivial operations (struct copies), consistency with linting rules is valued across the codebase.🔎 Optional enhancement for consistency
func (ctx *ExecutionContext) WithFile(file string) *ExecutionContext { + defer perf.Track(nil, "function.ExecutionContext.WithFile")() + newCtx := *ctx newCtx.File = file return &newCtx } func (ctx *ExecutionContext) WithBaseDir(baseDir string) *ExecutionContext { + defer perf.Track(nil, "function.ExecutionContext.WithBaseDir")() + newCtx := *ctx newCtx.BaseDir = baseDir return &newCtx } func (ctx *ExecutionContext) WithStackInfo(stackInfo *schema.ConfigAndStacksInfo) *ExecutionContext { + defer perf.Track(nil, "function.ExecutionContext.WithStackInfo")() + newCtx := *ctx newCtx.StackInfo = stackInfo return &newCtx }
📜 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 (19)
pkg/aws/identity/identity.gopkg/aws/identity/mock_identity.gopkg/function/context.gopkg/function/env.gopkg/function/env_test.gopkg/function/exec.gopkg/function/exec_test.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/phase.gopkg/function/phase_test.gopkg/function/random_test.gopkg/function/store_test.gopkg/function/tags.gopkg/function/tags_test.gopkg/store/mock_store.gopkg/store/store.gopkg/yaml/types_test.gotools/lintroller/rule_perf_track.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/function/phase_test.go
- pkg/function/tags_test.go
- pkg/yaml/types_test.go
- pkg/function/env_test.go
🧰 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: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in 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 for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/function/random_test.gotools/lintroller/rule_perf_track.gopkg/function/git_root_test.gopkg/aws/identity/mock_identity.gopkg/function/phase.gopkg/store/mock_store.gopkg/store/store.gopkg/function/tags.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/exec.gopkg/function/context.gopkg/function/env.gopkg/function/store_test.gopkg/aws/identity/identity.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: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/function/random_test.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/store_test.go
🧠 Learnings (56)
📓 Common learnings
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
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: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Avoid utils package bloat; don't add new functions to pkg/utils/; create purpose-built packages instead (examples: pkg/store/, pkg/git/, pkg/pro/, pkg/filesystem/)
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
📚 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:
pkg/function/random_test.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/function/random_test.gotools/lintroller/rule_perf_track.gopkg/function/git_root_test.gopkg/function/tags.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/store_test.gopkg/aws/identity/identity.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/function/random_test.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Applied to files:
pkg/function/random_test.gopkg/function/git_root_test.gopkg/aws/identity/mock_identity.gopkg/store/mock_store.gopkg/function/include_test.gopkg/function/store_test.gopkg/aws/identity/identity.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:
pkg/function/random_test.gopkg/function/git_root_test.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/store_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:
pkg/function/random_test.gopkg/aws/identity/mock_identity.gopkg/store/mock_store.gopkg/store/store.gopkg/function/store_test.gopkg/aws/identity/identity.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Define interfaces for all major functionality and use dependency injection for testability; generate mocks with go.uber.org/mock/mockgen
Applied to files:
pkg/function/random_test.gopkg/aws/identity/mock_identity.gopkg/store/mock_store.gopkg/store/store.gopkg/function/store_test.gopkg/aws/identity/identity.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:
pkg/function/random_test.gotools/lintroller/rule_perf_track.gopkg/function/exec_test.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:
pkg/function/random_test.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
pkg/function/random_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 : 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:
pkg/function/random_test.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:
pkg/function/random_test.gopkg/function/tags.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
pkg/function/random_test.gopkg/function/exec_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Applied to files:
pkg/function/random_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/function/random_test.gotools/lintroller/rule_perf_track.gopkg/function/git_root_test.gopkg/aws/identity/mock_identity.gopkg/function/phase.gopkg/store/mock_store.gopkg/store/store.gopkg/function/tags.gopkg/function/include_test.gopkg/function/exec_test.gopkg/function/exec.gopkg/function/context.gopkg/function/env.gopkg/function/store_test.gopkg/aws/identity/identity.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
tools/lintroller/rule_perf_track.gopkg/function/tags.gopkg/function/context.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:
tools/lintroller/rule_perf_track.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:
tools/lintroller/rule_perf_track.gopkg/function/context.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
tools/lintroller/rule_perf_track.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:
tools/lintroller/rule_perf_track.gopkg/function/tags.go
📚 Learning: 2025-10-02T19:17:51.630Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1504
File: pkg/profiler/profiler.go:20-31
Timestamp: 2025-10-02T19:17:51.630Z
Learning: In pkg/profiler/profiler.go, profiler-specific errors (ErrUnsupportedProfileType, ErrStartCPUProfile, ErrStartTraceProfile, ErrCreateProfileFile) must remain local and cannot be moved to errors/errors.go due to an import cycle: pkg/profiler → errors → pkg/schema → pkg/profiler. This is a valid exception to the centralized errors policy.
Applied to files:
tools/lintroller/rule_perf_track.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to cmd/**/*_test.go : ALWAYS use cmd.NewTestKit(t) for cmd tests to auto-clean RootCmd state (flags, args)
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
pkg/function/git_root_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:
pkg/function/git_root_test.go
📚 Learning: 2025-02-05T11:10:51.031Z
Learnt from: mss
Repo: cloudposse/atmos PR: 1024
File: internal/exec/go_getter_utils.go:31-33
Timestamp: 2025-02-05T11:10:51.031Z
Learning: The path traversal check in `ValidateURI` function in `internal/exec/go_getter_utils.go` is intentionally kept despite potentially blocking valid Git URLs, as this validation is planned to be addressed in a separate ticket.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/function/git_root_test.go
📚 Learning: 2024-10-22T23:00:20.627Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
Applied to files:
pkg/function/git_root_test.gopkg/function/exec_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Applied to files:
pkg/aws/identity/mock_identity.gopkg/store/mock_store.gopkg/store/store.gopkg/function/store_test.gopkg/aws/identity/identity.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Follow multi-provider registry pattern: define interface, implement per provider, register implementations, generate mocks (example: pkg/store/)
Applied to files:
pkg/store/mock_store.gopkg/store/store.gopkg/function/store_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Organize imports in 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:
pkg/function/tags.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/function/tags.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
pkg/function/tags.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:
pkg/function/tags.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 : Document all exported functions, types, and methods following Go's documentation conventions
Applied to files:
pkg/function/tags.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:
pkg/function/tags.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
pkg/function/tags.gopkg/function/exec_test.go
📚 Learning: 2025-06-07T19:27:40.807Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: pkg/utils/highlight_utils.go:0-0
Timestamp: 2025-06-07T19:27:40.807Z
Learning: In pkg/utils/highlight_utils.go, the global variable `isTermPresent` that caches terminal detection at package initialization is an intentional design choice and should not be changed to per-call detection.
Applied to files:
pkg/function/tags.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:
pkg/function/tags.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
pkg/function/tags.gopkg/function/exec_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 .github/workflows/*.{yml,yaml} : Configure CI to run unit tests, integration tests, golangci-lint, and coverage reporting on all pull requests
Applied to files:
pkg/function/include_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Keep files small and focused (<600 lines); one cmd/impl per file; co-locate tests; never use //revive:disable:file-length-limit
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
pkg/function/exec_test.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:
pkg/function/exec_test.go
📚 Learning: 2025-01-25T03:59:47.777Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:220-248
Timestamp: 2025-01-25T03:59:47.777Z
Learning: In the `downloadSource` function in `internal/exec/docs_generate.go`, the code is intentionally kept concise without additional security measures like content validation, download size limits, or domain validation to maintain simplicity and focus on core functionality.
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/function/env.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:
pkg/function/env.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use ATMOS_ prefix for environment variables with viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")
Applied to files:
pkg/function/env.go
📚 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:
pkg/function/env.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/aws/identity/identity.go
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
pkg/aws/identity/identity.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:
pkg/aws/identity/identity.go
📚 Learning: 2025-11-10T20:03:56.875Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1775
File: pkg/auth/providers/aws/sso_provisioning.go:40-79
Timestamp: 2025-11-10T20:03:56.875Z
Learning: In the Atmos AWS SSO provider (pkg/auth/providers/aws/sso_provisioning.go), the OAuth access token from the AWS SSO device flow is intentionally stored in the `AccessKeyID` field of `AWSCredentials` during authentication. This token is then extracted and used for ListAccounts and ListAccountRoles API calls during identity provisioning. This design reuses the existing `AWSCredentials` type for token transport rather than creating a separate credential type.
Applied to files:
pkg/aws/identity/identity.go
🧬 Code graph analysis (8)
pkg/function/random_test.go (3)
pkg/function/random.go (1)
NewRandomFunction(28-38)pkg/function/tags.go (1)
TagRandom(42-42)pkg/function/phase.go (2)
PreMerge(9-9)Phase(4-4)
pkg/aws/identity/mock_identity.go (2)
pkg/aws/identity/identity.go (2)
GetCallerIdentity(178-216)CallerIdentity(21-26)pkg/schema/schema.go (2)
AtmosConfiguration(54-99)AWSAuthContext(629-644)
pkg/function/tags.go (1)
pkg/perf/perf.go (1)
Track(121-138)
pkg/function/include_test.go (5)
pkg/function/function.go (1)
Function(11-23)pkg/function/include.go (2)
NewIncludeFunction(17-27)NewIncludeRawFunction(58-68)pkg/function/errors.go (1)
ErrSpecialYAMLHandling(23-23)pkg/function/tags.go (2)
TagInclude(33-33)TagIncludeRaw(36-36)pkg/function/phase.go (2)
PreMerge(9-9)Phase(4-4)
pkg/function/exec_test.go (3)
pkg/function/exec.go (1)
NewExecFunction(21-31)pkg/function/tags.go (1)
TagExec(12-12)pkg/function/phase.go (2)
PreMerge(9-9)Phase(4-4)
pkg/function/exec.go (8)
pkg/function/function.go (1)
BaseFunction(27-31)pkg/perf/perf.go (1)
Track(121-138)pkg/function/tags.go (1)
TagExec(12-12)pkg/function/phase.go (1)
PreMerge(9-9)pkg/function/resolution/context.go (1)
Context(24-27)pkg/function/context.go (1)
ExecutionContext(10-28)pkg/function/errors.go (2)
ErrInvalidArguments(14-14)ErrExecutionFailed(17-17)pkg/utils/shell_utils.go (1)
ExecuteShellAndReturnOutput(33-62)
pkg/function/context.go (2)
pkg/schema/schema.go (2)
AtmosConfiguration(54-99)ConfigAndStacksInfo(667-763)pkg/perf/perf.go (1)
Track(121-138)
pkg/aws/identity/identity.go (3)
pkg/perf/perf.go (1)
Track(121-138)pkg/logger/log.go (1)
Debug(24-26)errors/errors.go (1)
ErrAwsGetCallerIdentity(95-95)
⏰ 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 (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (33)
pkg/function/git_root_test.go (2)
15-22: Skip logic properly addresses previous concerns.The git availability and repository checks make this test safe for CI environments. The implementation correctly handles both missing git binary and non-repository contexts with clear skip messages.
44-53: Metadata test looks solid.The test properly validates the public API surface with clear assertions. The implementation is straightforward and tests the expected behavior.
tools/lintroller/rule_perf_track.go (2)
118-120: Good defensive guard.Prevents nil dereference on line 121. Returning early when package info is unavailable is the right behavior.
76-77: Verified: excluded receivers are correctly identified as trivial operations.The
ExecutionContext.With*mutators are O(1) struct copies with single field assignments, andPhase.String()is a trivial switch-based formatter. Both fit the exclusion criteria for perf tracking—adding overhead that exceeds actual method cost provides no actionable data.pkg/aws/identity/mock_identity.go (1)
1-57: LGTM! Generated mock is correct.The mock file is properly generated and matches the Getter interface. The past concern about the destination filename has been resolved—the directive now correctly generates
mock_identity.go.pkg/aws/identity/identity.go (3)
28-40: go:generate directive is now correct.The directive has been fixed to use
-destination=mock_identity.go, which aligns with codebase patterns. This resolves the previously flagged issue.
122-162: Solid caching implementation with thread-safe double-checked locking.The caching logic correctly handles concurrent access with RLock for reads and Lock for writes, caches both successful results and errors, and uses proper cache key generation per auth context.
251-317: Auth context integration looks good.The LoadConfigWithAuth function properly handles Atmos-managed credentials when auth context is provided while maintaining backward compatibility with standard AWS SDK credential resolution.
pkg/function/store_test.go (3)
16-136: Excellent test coverage with proper table-driven approach.The test suite comprehensively covers StoreFunction behavior including edge cases, error handling, and nil context validation. Tests use gomock-generated mocks and follow all coding guidelines.
154-265: StoreGetFunction tests are well-structured.Tests properly validate the StoreGetFunction behavior with comprehensive scenarios, including default value handling, query piping, and error cases. Good use of errors.Is for error checking.
267-495: Thorough parameter parsing test coverage.The parsing tests validate all forms of store command syntax including pipe options (default, query), edge cases, and error conditions. Helper function tests complete the coverage.
pkg/store/mock_store.go (1)
1-84: LGTM! Mock correctly implements Store interface.The generated mock properly includes all Store methods (Get, GetKey, Set) and follows gomock conventions. Aligns with the updated Store interface.
pkg/function/include_test.go (2)
11-41: Well-structured test for special YAML handling.The table-driven test correctly verifies that both
IncludeandIncludeRawreturnErrSpecialYAMLHandlingusingerrors.Is(), with appropriate message validation. Good coverage for these special-case functions.
43-72: Complete metadata validation.The test comprehensively validates constructor behavior and metadata methods for both Include variants, ensuring correct Name, Phase, and Aliases values.
pkg/function/random_test.go (5)
12-67: Comprehensive error case coverage with correct sentinel error checking.The test properly uses
errors.Is()to verifyErrInvalidArguments(line 63) and covers all invalid input scenarios. The past review feedback has been addressed.
69-81: Good boundary value testing.Multiple iterations with min=0, max=1 properly validate the randomness and range constraints.
83-93: Negative range handling verified.The test ensures the function correctly handles negative min values and returns results within the specified range.
95-101: Constructor test follows established pattern.Metadata validation is complete and consistent with other function tests.
103-116: Internal helper test with correct error validation.The test properly validates both success and error cases, using
errors.Is()(line 115) for sentinel error checking as required by guidelines.pkg/function/phase.go (1)
3-14: Clean phase type definition with helpful documentation.The Phase type and constants are well-documented with examples of which functions belong to each phase. Using
iotais idiomatic Go.pkg/function/exec_test.go (3)
11-29: Invalid argument handling properly tested.The test correctly uses
errors.Is()to verifyErrInvalidArgumentsfor empty and whitespace-only inputs.
31-80: Comprehensive output parsing validation.The test thoroughly covers different output formats (plain string, JSON object, JSON array, non-JSON). The checkFunc pattern is clean and flexible. While these execute actual shell commands (making them integration tests), using
echois reliable enough for this case.
82-88: Metadata validation complete.Standard constructor test verifying all interface methods.
pkg/function/exec.go (1)
15-31: Constructor follows established pattern.The ExecFunction type and constructor are consistent with other function implementations in the package.
pkg/function/env.go (4)
14-30: Constructor follows established pattern.Clean implementation consistent with other function types in the package.
32-52: Robust argument parsing with proper error handling.The function correctly validates inputs, handles both single and double argument cases, and wraps errors with the sentinel
ErrInvalidArgumentsplus context.
54-67: Well-designed context lookup with proper nil safety.The function safely checks for nil contexts and returns both value and found status, allowing proper distinction between "not set" and "set to empty string."
69-99: Execute method implements proper precedence hierarchy.The implementation correctly prioritizes:
- Stack manifest env section (context)
- OS environment variables
- Provided default
- Empty string fallback
The use of
os.LookupEnvproperly distinguishes between unset and empty variables.pkg/function/context.go (1)
8-39: Well-designed context type with clear documentation.The ExecutionContext provides all necessary runtime information for function execution. The constructor properly initializes core fields and uses the
atmosConfigparameter for performance tracking.pkg/function/tags.go (4)
7-61: Comprehensive tag constants with excellent documentation.All tag constants are well-documented with clear descriptions of their purposes. The organization and naming are consistent.
63-85: AllTags function properly encapsulates tag list.Returns a new slice on each call, preventing external mutation of internal state. Includes performance tracking as required.
87-112: IsValidTag properly encapsulates tag validation.The previous concern about a mutable public TagsMap has been addressed. The map is now private, and
IsValidTag()provides safe O(1) lookup without exposing internal state.
114-129: YAML tag helpers are clean and defensive.
YAMLTagandFromYAMLTagprovide bidirectional conversion with proper handling of edge cases.FromYAMLTagsafely handles input both with and without the prefix.
|
These changes were released in v1.203.0-rc.1. |
|
These changes were released in v1.203.0-test.5. |
|
These changes were released in v1.203.0-test.10. |
2 participants
What
Create new packages for modularized function handling and YAML utilities:
Why
References
Foundation for PR #1842 which refactors YAML processing.
Consolidates AWS identity handling per request.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.