feat(stack): Add multi-format stack support with convert command#1842
feat(stack): Add multi-format stack support with convert command#1842
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (130)
📝 WalkthroughWalkthroughAdds multi-format stack support (YAML/JSON/HCL), a stack loader/registry and per-format loaders, a stack processor with phase-aware YAML/HCL functions, HCL eval integration, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as atmos CLI
participant LoaderReg as LoaderRegistry
participant Loader as FormatLoader
participant Processor as StackProcessor
participant Converter as StackConverter
participant Output as OutputHandler
User->>CLI: atmos stack convert file.yaml --to=hcl
CLI->>LoaderReg: select loader (by ext/content)
LoaderReg->>Loader: LoadWithMetadata(file bytes)
Loader-->>LoaderReg: []StackDocument
CLI->>Converter: convertStacksToFormat(docs, target=hcl)
Converter->>Converter: serialize docs -> HCL blocks
Converter-->>CLI: bytes
CLI->>Output: write file / stdout / dry-run preview
Output-->>User: result
sequenceDiagram
participant Processor as Processor
participant FuncReg as FunctionRegistry
participant Func as FunctionImpl
participant ExecCtx as ExecutionContext
Processor->>Processor: traverse data (maps/slices/strings)
Processor->>FuncReg: lookup function "env"/"include"/...
FuncReg-->>Processor: Function instance
Processor->>Func: Execute(ctx, args, execCtx)
Func->>ExecCtx: query Env / WorkingDir / SourceFile
ExecCtx-->>Func: value
Func-->>Processor: result
Processor->>Processor: replace token and continue
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/filetype/filetype.go (1)
258-262: Number conversion has unreachable code.The
if n, _ := ...; truepattern always evaluates to true, soreturn value.AsBigFloat()on line 262 is dead code.This should likely check for integer conversion success:
case cty.Number: - if n, _ := value.AsBigFloat().Int64(); true { + bf := value.AsBigFloat() + if bf.IsInt() { + n, _ := bf.Int64() return n } - return value.AsBigFloat() + f, _ := bf.Float64() + return f
🧹 Nitpick comments (44)
pkg/utils/file_extensions.go (1)
16-41: Centralized stack-extension helpers look good; minor perf nit only if this is hot.The added JSON/HCL constants and
StackConfigExtensions/IsStackConfigFilehelpers make extension handling much clearer, and the ordering reads well (YAML/YML → JSON/HCL → templates). IfIsStackConfigFileends up in a very hot path over large file lists, you could consider a package-level var for the slice or a smallmap[string]struct{}to avoid repeated allocations and linear scans—but that’s purely an optional micro-optimization.website/plugins/remark-stack-example/function-syntax.js (1)
152-172: parseTerraformArg mutates input parameter.Line 159 reassigns
argafter extracting stack parameter. While this works in JavaScript, it can be confusing. Consider using a local variable.function parseTerraformArg(arg) { const result = { component: '', output: '', stack: '' }; + let remainingArg = arg; // Extract stack= parameter if present. const stackMatch = arg.match(/\s+stack=(\S+)/); if (stackMatch) { result.stack = stackMatch[1]; - arg = arg.replace(stackMatch[0], '').trim(); + remainingArg = arg.replace(stackMatch[0], '').trim(); } // Parse component.output. - const parts = arg.split('.'); + const parts = remainingArg.split('.');docs/prd/file-handler-registry.md (3)
42-50: Add language specifiers to code blocks.Static analysis flagged these blocks for missing language tags. Since they're flow diagrams, use
textorplaintext.-``` +```text File → Read Bytes → Detect Format → Parse → YAML Node → Process Tags → Decode → map[string]any -``` +``` The goal is to change this to: -``` +```text File → Read Bytes → Registry Lookup → Handler.Parse() → map[string]any → Function Processing → Final Config -``` +```
56-110: Package structure diagram needs language tag.Use
textfor the directory tree.-``` +```text pkg/ ├── filehandler/ # Core file handler infrastructure
798-838: Dependency tree diagram needs language tag.Same issue as above.
-``` +```text pkg/filehandler/ ├── handler.go (no deps)pkg/stack/loader/defaults_test.go (1)
9-17: Consider table-driven test for consistency.The test correctly validates all supported extensions. For consistency with Go testing best practices and coding guidelines preference for table-driven tests, consider this refactor:
func TestSupportedExtensions(t *testing.T) { exts := SupportedExtensions() - assert.Contains(t, exts, ".yaml") - assert.Contains(t, exts, ".yml") - assert.Contains(t, exts, ".json") - assert.Contains(t, exts, ".hcl") - assert.Contains(t, exts, ".tf") + expected := []string{".yaml", ".yml", ".json", ".hcl", ".tf"} + + for _, ext := range expected { + assert.Contains(t, exts, ext, "expected extension %s to be supported", ext) + } + + assert.Equal(t, len(expected), len(exts), "unexpected number of extensions") }This approach makes it easier to add new extensions and validates the exact count.
website/docs/functions/hcl/atmos-template.mdx (1)
1-7: Verifysidebar_class_namevalue.The frontmatter uses
sidebar_class_name: command, but this is documenting an HCL function, not a CLI command. Consider using a more appropriate class name likefunctionif one exists in the site's styling, or removing it if unnecessary.--- title: "atmos::template" sidebar_position: 4 sidebar_label: "atmos::template" -sidebar_class_name: command +sidebar_class_name: function description: Evaluate Go templates in HCL stack configurations ---pkg/stack/multi_format_test.go (1)
33-53: Consider adding content validation.The tests verify no errors and correct result counts, which is a good start. For more robust coverage, consider validating that the parsed content contains expected keys or values to confirm the format was actually parsed correctly.
// Example: verify the stack contains expected components components, ok := mapResult["deploy/dev"].(map[string]any)["components"] assert.True(t, ok, "expected components key in result")pkg/filetype/filetype_hcl_functions_test.go (1)
53-60: Type assertions could cause test panics.These chained type assertions will panic if the structure doesn't match, making debugging harder. Consider using require with type checks or the two-value assertion form.
- components := resultMap["components"].(map[string]any) - terraform := components["terraform"].(map[string]any) - myapp := terraform["myapp"].(map[string]any) - vars := myapp["vars"].(map[string]any) + components, ok := resultMap["components"].(map[string]any) + require.True(t, ok, "components should be map[string]any") + terraform, ok := components["terraform"].(map[string]any) + require.True(t, ok, "terraform should be map[string]any") + myapp, ok := terraform["myapp"].(map[string]any) + require.True(t, ok, "myapp should be map[string]any") + vars, ok := myapp["vars"].(map[string]any) + require.True(t, ok, "vars should be map[string]any")website/src/components/StackExample/index.tsx (1)
35-45: Remove unused copy functionality or wire it up.The
copiedstate andhandleCopyfunction are defined but never invoked in the rendered JSX. Either connect this to copy buttons in the UI or remove it to keep the code lean.- const [copied, setCopied] = useState<string | null>(null); - - const handleCopy = async (format: string, content: string) => { - try { - await navigator.clipboard.writeText(content); - setCopied(format); - setTimeout(() => setCopied(null), 2000); - } catch (err) { - console.error('Failed to copy:', err); - } - }; - return (pkg/function/registry_test.go (1)
224-250: Consider using sync.WaitGroup for clearer concurrent test synchronization.The channel-based synchronization works, but
sync.WaitGroupwould make the concurrent test intent clearer and is the more idiomatic approach.+import "sync" func TestRegistryConcurrentAccess(t *testing.T) { r := NewRegistry() - done := make(chan bool) + var wg sync.WaitGroup // Concurrent registration. + wg.Add(1) go func() { + defer wg.Done() for i := 0; i < 100; i++ { fn := newMockFunction("env", nil, PreMerge) _ = r.Register(fn) _ = r.Unregister("env") } - done <- true }() // Concurrent reads. + wg.Add(1) go func() { + defer wg.Done() for i := 0; i < 100; i++ { _ = r.Has("env") _, _ = r.Get("env") _ = r.List() } - done <- true }() - <-done - <-done + wg.Wait() }pkg/function/errors.go (1)
13-17: Consider consolidating similar execution errors.
ErrFunctionExecutionandErrExecutionFailedappear semantically similar. If they serve different purposes, consider adding comments explaining when to use each. Otherwise, consolidate to a single error to reduce confusion.pkg/stack/defaults.go (1)
40-46: Consider caching the registry for repeated calls.
IsExtensionSupportedcreates a new registry on every invocation. For frequent lookups, this could be inefficient. If this becomes a hot path, consider a package-level lazy-initialized registry.That said, this is likely fine for current usage patterns.
pkg/stack/defaults_test.go (1)
104-124: Consider asserting the processed value.The integration test validates that processing succeeds without error, but doesn't verify the
!env AWS_REGIONresolved to"us-west-2". Adding this assertion would strengthen the test.// Test post-merge processing. result, err := p.ProcessPostMerge(context.Background(), stackCtx, data) require.NoError(t, err) assert.NotNil(t, result) + + // Verify the !env function resolved correctly. + resultMap, ok := result.(map[string]any) + require.True(t, ok) + assert.Equal(t, "us-west-2", resultMap["region"])pkg/function/context.go (2)
33-41: Consider removing perf tracking from trivial accessor.
GetEnvis a simple map lookup with nil checks. The performance tracking overhead likely exceeds the actual method cost, making the profiling data less useful.Based on learnings, perf.Track() should be excluded from trivial accessors/mutators like this where the tracking overhead would exceed the actual method cost.
Apply this diff:
func (c *ExecutionContext) GetEnv(key string) string { - defer perf.Track(nil, "function.ExecutionContext.GetEnv")() - if c == nil || c.Env == nil { return "" } return c.Env[key] }
43-52: Consider removing perf tracking from trivial accessor.
HasEnvis a simple map lookup with nil checks. The performance tracking overhead likely exceeds the actual method cost.Based on learnings, perf.Track() should be excluded from trivial accessors/mutators where the tracking overhead would exceed the actual method cost.
Apply this diff:
func (c *ExecutionContext) HasEnv(key string) bool { - defer perf.Track(nil, "function.ExecutionContext.HasEnv")() - if c == nil || c.Env == nil { return false } _, ok := c.Env[key] return ok }website/docs/functions/hcl/stdlib.mdx (1)
62-63: Consider relocatingreverse()to Collection Functions.Since
reverse()operates on lists/tuples, it would be more appropriately placed in the "Collection Functions" section rather than "String Functions."pkg/function/function.go (1)
30-35: Note: BaseFunction doesn't implement the full Function interface.This is intentional - it's designed for embedding. Concrete implementations must provide
Execute. Consider adding a doc comment clarifying this pattern for future maintainers.+// BaseFunction provides a base implementation for Function that can be embedded. +// Note: This type does not implement Execute, so it cannot be used directly as a Function. +// Embed it in concrete types that provide their own Execute implementation. type BaseFunction struct { FunctionName string FunctionAliases []string FunctionPhase Phase }pkg/function/hcl_test.go (1)
82-103: Consider verifying the actual cty types.The
expectedfield in the test struct captures expected type names but isn't used in assertions. This weakens the test's ability to catch regressions.for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := toCtyValue(tt.input) assert.NotNil(t, result) + assert.Equal(t, tt.expected, result.Type().FriendlyName()) }) }pkg/function/exec.go (1)
78-84: Consider trimming output before returning as string.Shell commands often include trailing newlines. When JSON parsing fails and you return the raw output, that newline persists. Trimming might provide cleaner results.
// Try to parse output as JSON, fall back to string. var decoded any if err := json.Unmarshal([]byte(output), &decoded); err != nil { - return output, nil + return strings.TrimSpace(output), nil }pkg/function/template.go (1)
54-103: Consider handling the ignored error for future-proofing.Line 74 ignores the error from
templateFn.Execute. Currently safe sinceExecutenever returns an error, but if that changes, errors will be silently dropped.case string: // Only process !template tags, leave other tags as-is. if strings.HasPrefix(v, TagTemplate) { args := strings.TrimPrefix(v, TagTemplate) - result, _ := templateFn.Execute(context.Background(), args, nil) + result, err := templateFn.Execute(context.Background(), args, nil) + if err != nil { + // Log or handle; for now return original value on error. + return v + } return result } return vwebsite/plugins/remark-stack-example/index.js (1)
47-119: Solid plugin implementation.Good approach with:
- Collecting replacements first, then applying in reverse order to preserve indices.
- Graceful error handling that preserves the original block on failure.
- Clean attribute construction for the MDX JSX element.
One minor consideration: the
optionsparameter is declared but unused. If it's reserved for future extensibility, a brief comment noting that would help.pkg/function/resolution/context_test.go (1)
588-602: Redundant assertions.Lines 598 and 601 test the same thing -
errors.Isandassert.ErrorIsare equivalent checks. One is sufficient.// Should be able to check with errors.Is. assert.True(t, errors.Is(err, ErrCycleDetected)) - - // Should contain wrapped error. - assert.ErrorIs(t, err, ErrCycleDetected)pkg/function/git_root.go (1)
51-74: Context parameter is accepted but unused.The
ctx context.Contextparameter is passed toExecutebut never used for cancellation or deadlines. If this is for interface compliance, that's fine. Otherwise, consider propagating it tofindGitRootfor potential long-running git operations.pkg/function/env.go (1)
92-150: Quote handling has a subtle edge case.Nested quotes of different types (e.g.,
VAR "value with 'apostrophe'") work correctly since you trackquoteChar. However, escaped quotes within the same type aren't handled -VAR "value with \"escaped\""would fail.If this is a known limitation, a brief doc comment noting it would help users.
// stringSplitter handles splitting strings by delimiter with quote support. +// Note: Escaped quotes within the same quote type are not supported. +// Use alternating quote styles for embedded quotes: VAR "value with 'quotes'" type stringSplitter struct {pkg/function/hcl.go (2)
135-171: Silent data loss in default case.When
toCtyValueencounters an unsupported type, it returns an empty string without any indication. This could mask issues. Consider logging a warning or returning the%vrepresentation.default: - return cty.StringVal("") + return cty.StringVal(fmt.Sprintf("%v", val)) }
173-185: Simplify withstrings.ReplaceAll.The manual rune iteration can be replaced with a standard library call.
+import "strings" + // normalizeHCLName converts an Atmos function name to a valid HCL identifier. // Replaces hyphens with underscores since HCL doesn't allow hyphens in identifiers. func normalizeHCLName(name string) string { - result := "" - for _, c := range name { - if c == '-' { - result += "_" - } else { - result += string(c) - } - } - return result + return strings.ReplaceAll(name, "-", "_") }pkg/stack/loader/hcl/loader_test.go (2)
137-168: LoadWithMetadata test covers key aspects.Position extraction and source file attribution verified. Consider adding a test for when
WithSourceFileis omitted to ensure default behavior.
526-537: Use assert.Contains for consistency.Line 536 uses
strings.Containsdirectly. For consistency with the rest of the test file, prefer using testify's assertion.- assert.True(t, strings.Contains(m["script"].(string), "echo")) + assert.Contains(t, m["script"].(string), "echo")pkg/function/registry.go (2)
107-121: GetByPhase returns non-deterministic order.Map iteration order in Go is randomized. If callers depend on consistent ordering (e.g., for reproducible execution), consider sorting the result.
+import "sort" + func (r *Registry) GetByPhase(phase Phase) []Function { defer perf.Track(nil, "function.Registry.GetByPhase")() r.mu.RLock() defer r.mu.RUnlock() var result []Function for _, fn := range r.functions { if fn.Phase() == phase { result = append(result, fn) } } + sort.Slice(result, func(i, j int) bool { + return result[i].Name() < result[j].Name() + }) return result }
123-135: List also returns non-deterministic order.Same consideration as GetByPhase. Sorting would provide stable output for debugging and testing.
pkg/stack/processor/context.go (1)
116-136: Clone comment says "shallow copy" but behavior is mixed.The Skip slice is deep copied, but the embedded
ExecutionContextpointer is shared. If the original's ExecutionContext is modified, the clone sees changes. Consider clarifying in the comment or cloning ExecutionContext if isolation is needed.-// Clone creates a shallow copy of the context. +// Clone creates a copy of the context with a copied Skip slice. +// Note: The embedded ExecutionContext is shared, not cloned.pkg/stack/loader/yaml/loader.go (3)
21-29: Unbounded per-key locks map may grow indefinitely.The
locksmap accumulates entries over time but is never pruned. For long-running processes parsing many unique files, this could lead to memory growth. Consider cleaning up locks after use or using a sync.Pool pattern.+// cleanupLock removes a lock after use. +func (l *Loader) cleanupLock(key string) { + l.locksMu.Lock() + defer l.locksMu.Unlock() + delete(l.locks, key) +}Then call it after caching the result in
parseWithLocking.
126-135: Empty cache key bypasses caching entirely.When
file == ""orcontentis empty,generateCacheKeyreturns"", and subsequent code skips caching. This is intentional behavior but worth documenting explicitly in the function comment.// generateCacheKey creates a cache key from file path and content hash. +// Returns empty string if file or content is empty, which disables caching. func (l *Loader) generateCacheKey(file string, content []byte) string {
298-306: ClearCache doesn't clear the locks map.When clearing the cache, the
locksmap retains all entries. Consider clearing both for complete reset.func (l *Loader) ClearCache() { defer perf.Track(nil, "yaml.Loader.ClearCache")() l.cacheMu.Lock() defer l.cacheMu.Unlock() l.cache = make(map[string]*cacheEntry) + + l.locksMu.Lock() + l.locks = make(map[string]*sync.Mutex) + l.locksMu.Unlock() }pkg/stack/loader/json/loader.go (1)
252-258: Array elements receive placeholder positions.Array elements at lines 256 always get
Line: 1, Column: 1. This is noted as a placeholder but may confuse consumers expecting real positions.Consider using a sentinel value like
Line: 0, Column: 0to indicate "position unknown" or document this behavior in the Metadata struct.internal/exec/stack_convert.go (3)
34-38: Local error definitions instead of centralized errors.Per coding guidelines, errors should be defined in
errors/errors.goand used viaerrors.Is(). These local errors work but break from the pattern.Consider moving these to
errors/errors.go:// In errors/errors.go var ( ErrUnsupportedFormat = errors.New("unsupported format") ErrConversionFailed = errors.New("conversion failed") ErrUnknownSourceFormat = errors.New("unknown source format") )
86-89: Error not wrapped with static error.Line 88 creates a dynamic error string. Per guidelines, wrap with a static error.
- if err := os.WriteFile(outputPath, []byte(output), filePermissions); err != nil { - return fmt.Errorf("failed to write output file: %w", err) - } + if err := os.WriteFile(outputPath, []byte(output), filePermissions); err != nil { + return fmt.Errorf("%w: %w", ErrConversionFailed, err) + }
330-371: goToCtyForHCL duplicates utils.GoToCty with minor differences.This function largely mirrors
utils.GoToCtybut usescty.TupleValfor arrays instead of potentiallycty.ListVal. The fallback toutils.GoToCtyat line 369 covers unknown types. Consider documenting why this separate implementation exists.Add a comment explaining the difference from
utils.GoToCty:// goToCtyForHCL converts Go types to cty.Value, handling special cases for HCL. +// Unlike utils.GoToCty, this uses TupleVal for arrays to allow mixed types, +// which is more natural for heterogeneous HCL configurations.website/plugins/remark-stack-example/converter.js (1)
150-159: Unused loop variableindex.The
indexparameter is declared but never used in this forEach callback.- keys.forEach((key, index) => { + keys.forEach((key) => {pkg/stack/processor/processor.go (3)
34-42: Consider removing perf.Track from trivial accessors.
FunctionRegistry()is a simple field access. The perf.Track overhead may exceed the method's execution time. The coding guidelines say to add perf.Track to public functions, but for trivial accessors this may be counterproductive.
175-199: Consider removing perf.Track from parseFunction.This method is called once per string in the data tree during processing. The tracking overhead could accumulate significantly for large configurations. The actual parsing logic is minimal.
240-262: Inefficient repeated function detection.
HasPreMergeFunctionsandHasPostMergeFunctionseach callDetectFunctions, meaning the data is traversed twice if both are called. Consider a combined method if both checks are commonly needed together.internal/exec/yaml_func_resolution_context.go (1)
164-174: Visited() exposes internal map directly.Returning
ctx.inner.Visitedallows callers to mutate the internal state. Consider returning a copy for safety, or document that this is intentional for performance.func (ctx *ResolutionContext) Visited() map[string]bool { defer perf.Track(nil, "exec.ResolutionContext.Visited")() if ctx == nil || ctx.inner == nil { return nil } - return ctx.inner.Visited + // Return a copy to prevent external mutation. + visited := make(map[string]bool, len(ctx.inner.Visited)) + for k, v := range ctx.inner.Visited { + visited[k] = v + } + return visited }
|
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. |
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Move errors from pkg/stack/loader/errors.go and pkg/stack/processor/errors.go to the central errors/errors.go package for consistent error handling. Changes: - Add stack loader errors: ErrUnsupportedFormat, ErrLoaderParseFailed, ErrEncodeFailed, ErrLoaderNotFound, ErrDuplicateLoader - Add stack processor errors: ErrProcessingFailed, ErrProcessorFunctionNotFound, ErrProcessorInvalidData, ErrSkippedFunction, ErrProcessorCycleDetected, ErrNilProcessor, ErrProcessorNilContext - Update all loader implementations (JSON, HCL, YAML) to use errUtils - Update registry and processor to use centralized errors - Delete pkg/stack/loader/errors.go and pkg/stack/processor/errors.go - Fix godot lint issue in json/loader.go comment - Add perf.Track to phase.go String() method - Address CodeRabbit review feedback on PR #1842 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
internal/exec/yaml_func_resolution_context.go (2)
62-69: Key format ambiguity already flagged.This concern about hyphenated names in the key format has been raised in previous reviews.
120-135: parseNodeKey ambiguity already flagged.The split-on-first-hyphen issue has been raised in previous reviews. The extensive comment acknowledges the limitation but the parsing logic remains fragile.
pkg/stack/loader/hcl/loader.go (2)
235-251: Limitation documented appropriately.The comment addresses the unbounded growth concern from past reviews. Acceptable for typical Atmos session sizes.
509-540: Silent skip behavior documented.The detailed NOTE comment addresses the past review concern. Intentional design for partial extraction when some attributes reference undefined variables.
🧹 Nitpick comments (15)
pkg/filetype/filetype.go (1)
176-176: Consider refactoring instead of nolint directives.Per coding guidelines,
nolintforgocognit,nestif, and cyclomatic complexity should be avoided in favor of refactoring. The function could potentially be split:
- Extract block-without-labels handling into a helper
- Extract block-with-labels handling into another helper
- Keep the main function as orchestration
That said, the logic is coherent and handles a genuinely complex parsing task.
pkg/function/hcl.go (3)
78-78: Consider propagating context for cancellation support.Using
context.Background()here means long-running Atmos functions can't be cancelled. If HCL evaluation needs timeout support, consider threading context throughHCLFunctionsandwrapAtmosFunction.
122-124: Default case silently returns empty string.Unknown types get converted to
""without warning. This could mask bugs if an Atmos function returns an unexpected type. Consider logging or returning an error.default: + // Log unexpected type for debugging. return cty.StringVal("")
127-139: Usestrings.ReplaceAllfor efficiency.Character-by-character string concatenation is O(n²). The standard library provides a cleaner one-liner.
+import "strings" + // normalizeHCLName converts an Atmos function name to a valid HCL identifier. // Replaces hyphens with underscores since HCL doesn't allow hyphens in identifiers. func normalizeHCLName(name string) string { - result := "" - for _, c := range name { - if c == '-' { - result += "_" - } else { - result += string(c) - } - } - return result + return strings.ReplaceAll(name, "-", "_") }internal/exec/yaml_func_resolution_context.go (1)
52-60: Consider adding perf.Track for consistency.While this is just a passthrough, adding
defer perf.Track(nil, "exec.scopedResolutionContext")()would maintain consistency with other public functions in the file.pkg/function/exec_test.go (2)
30-95: Strengthen error assertions in the table-driven test.The table and flow are good and cover the main behaviors. For the
"execution failure"case, consider also asserting the specific sentinel (e.g.ErrExecutionFailed) withassert.ErrorIs, so we lock in the error contract rather than just “some error”. For example:tests := []struct { name string args string output string execError error expected any expectError bool + expectedErr error }{ { name: "simple string output", @@ { name: "execution failure", args: "failing-command", execError: errors.New("command failed"), expectError: true, + expectedErr: ErrExecutionFailed, }, @@ - if tt.expectError { - require.Error(t, err) - return - } + if tt.expectError { + require.Error(t, err) + if tt.expectedErr != nil { + assert.ErrorIs(t, err, tt.expectedErr) + } + return + }This stays table-driven and aligns with the project guideline to use
assert.ErrorIsfor our own errors. As per coding guidelines, this helps keep behavior-focused tests resilient to implementation changes.
105-123: Assert that context-based execution succeeds.
TestExecFunctionWithContextnicely verifiesWorkingDirandEnvpropagation viacapturingExecutor. To also guard against regressions whereExecutemight start returning an error in this path, you could assert success explicitly:- _, _ = fn.Execute(context.Background(), "test command", execCtx) + _, err := fn.Execute(context.Background(), "test command", execCtx) + require.NoError(t, err)This keeps the test behavior-focused while tightening expectations on the happy path.
cmd/stack/convert.go (1)
36-50: Avoid silently swallowing all Atmos config initialization errorsRight now any error from
cfg.InitCliConfigis treated the same and replaced with an emptyschema.AtmosConfiguration{}. That makes sense for the “config is optional” story, but it also hides real issues (e.g., malformedatmos.yaml) and preventsExecuteStackConvertfrom knowing whether there was no config vs a broken config.Consider tightening this a bit by either:
- Distinguishing “config not found” from “invalid config” using the existing sentinel errors in the config/errors packages and only falling back to empty config for the “not found” case, or
- At least emitting a warning when config loading fails so users get a hint that their Atmos config is being ignored.
This keeps the command usable without config while making configuration problems much easier to diagnose.
As per coding guidelines, this would give clearer user-facing error semantics while still treating config as optional for this command.
pkg/stack/loader/yaml/loader.go (3)
18-19: Consider using the centralized error wrap format.
errWrapduplicatesErrWrapFormatfromerrors/errors.go. UseerrUtils.ErrWrapFormatfor consistency across the codebase.-// errWrap is the error wrapping format string. -const errWrap = "%w: %w"Then update usages to
errUtils.ErrWrapFormat.
162-173: Per-key locks grow unbounded.Same pattern as HCL loader. The
locksmap accumulates without cleanup. For typical Atmos sessions this is bounded by unique files, but consider documenting this limitation or adding a comment similar to the HCL loader (lines 236-240 in hcl/loader.go).
299-307: ClearCache doesn't clear the locks map.When clearing the cache, you may also want to clear the
locksmap to prevent stale per-key locks from accumulating. Current behavior is safe but could lead to memory growth over time ifClearCacheis called repeatedly with different file sets.func (l *Loader) ClearCache() { defer perf.Track(nil, "yaml.Loader.ClearCache")() l.cacheMu.Lock() defer l.cacheMu.Unlock() l.cache = make(map[string]*cacheEntry) + + l.locksMu.Lock() + defer l.locksMu.Unlock() + l.locks = make(map[string]*sync.Mutex) }pkg/stack/loader/hcl/loader.go (2)
144-166:json.Numbertype not handled ingoToCty.If data is loaded via JSON loader (which preserves
json.Number), then converted to HCL, this will returnErrInvalidType. Consider handlingjson.Number:+ case json.Number: + if i, err := v.Int64(); err == nil { + return cty.NumberIntVal(i), nil + } + f, _ := v.Float64() + return cty.NumberFloatVal(f), nil
419-447: Position placeholders are hardcoded.Lines 424 and 439 set
{Line: 1, Column: 1}for nested elements. This is a known HCL library limitation. Consider documenting this in a comment.pkg/stack/processor/processor_test.go (1)
267-276: Context cancellation test could be more specific.Consider verifying the error type:
_, err := p.ProcessPreMerge(ctx, map[string]any{"key": "value"}, "test.yaml") - assert.Error(t, err) + assert.ErrorIs(t, err, context.Canceled)pkg/stack/processor/processor.go (1)
241-263: Add nil guards forProcessorandfunctionsin phase-specific helpers.
HasPreMergeFunctionsandHasPostMergeFunctionsassumepandp.functionsare non-nil, but other methods likeFunctionRegistry,LoaderRegistry, andDetectFunctionsare explicitly nil-safe. If aProcessoris constructed with a nil function registry (or a nil receiver is used by mistake), these helpers will panic onp.functions.Get.To keep behavior consistent and defensive, consider early-returning
falsewhenp == nilorp.functions == nil:func (p *Processor) HasPreMergeFunctions(data any) bool { defer perf.Track(nil, "processor.Processor.HasPreMergeFunctions")() + if p == nil || p.functions == nil { + return false + } + for _, name := range p.DetectFunctions(data) { if fn, err := p.functions.Get(name); err == nil && fn.Phase() == function.PreMerge { return true } } return false } func (p *Processor) HasPostMergeFunctions(data any) bool { defer perf.Track(nil, "processor.Processor.HasPostMergeFunctions")() + if p == nil || p.functions == nil { + return false + } + for _, name := range p.DetectFunctions(data) { if fn, err := p.functions.Get(name); err == nil && fn.Phase() == function.PostMerge { return true } } return false }This matches the existing nil-tolerant pattern elsewhere and avoids surprising panics in edge cases.
📜 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 (17)
cmd/stack/convert.go(1 hunks)errors/errors.go(1 hunks)internal/exec/yaml_func_resolution_context.go(4 hunks)pkg/filetype/filetype.go(3 hunks)pkg/function/exec_test.go(1 hunks)pkg/function/hcl.go(1 hunks)pkg/stack/loader/hcl/loader.go(1 hunks)pkg/stack/loader/hcl/loader_test.go(1 hunks)pkg/stack/loader/json/loader.go(1 hunks)pkg/stack/loader/json/loader_test.go(1 hunks)pkg/stack/loader/registry.go(1 hunks)pkg/stack/loader/registry_test.go(1 hunks)pkg/stack/loader/yaml/loader.go(1 hunks)pkg/stack/loader/yaml/loader_test.go(1 hunks)pkg/stack/processor/processor.go(1 hunks)pkg/stack/processor/processor_test.go(1 hunks)website/docs/functions/hcl/stdlib.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/stack/loader/hcl/loader_test.go
- pkg/stack/loader/yaml/loader_test.go
- pkg/stack/loader/registry_test.go
- website/docs/functions/hcl/stdlib.mdx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.goerrors/errors.gopkg/function/hcl.gopkg/filetype/filetype.gopkg/stack/loader/registry.gopkg/stack/loader/yaml/loader.gocmd/stack/convert.gopkg/function/exec_test.gopkg/stack/loader/hcl/loader.gopkg/stack/loader/json/loader.gointernal/exec/yaml_func_resolution_context.gopkg/stack/processor/processor.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Useassert.ErrorIs(t, err, ErrSentinel)for checking our own errors and stdlib errors. Only use string matching for third-party errors
NEVER useassert.Contains(err.Error(), ...)for error checking. ALWAYS useassert.ErrorIs()instead. NEVER use string comparison:err.Error() == "..."orstrings.Contains(err.Error(), ...)
Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoidos.Exit,CheckErrorPrintAndExit, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Usego.uber.org/mock/mockgenwith//go:generatedirectives for mock generation. Never create manual mocks. Pattern://go:generate go run go.uber.org/mock/mockgen@latest -source=filename.go -destination=mock_filename_test.go
Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.gopkg/function/exec_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Business logic packages must follow the Service-Oriented Architecture pattern with: one Service struct per domain, Provider interfaces for classes of problems, default implementations, mock implementations, and dependency injection for testability
Use interface-driven design: define interfaces for all major functionality, use dependency injection for testability, generate mocks withgo.uber.org/mock/mockgenwith//go:generatedirectives, avoid integration tests by mocking external dependencies
Create focused purpose-built packages inpkg/for new functionality, each with clear responsibility. DO NOT add new functions topkg/utils/orinternal/exec/. Each package should have focused business logic with tests
Implement registry pattern for extensibility and plugin-like architecture. Define interfaces for provider implementations, register implementations in registry, generate mocks with mockgen. Examples: Command Registry incmd/internal/registry.go, Store Registry inpkg/store/registry.go
Files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.gopkg/function/hcl.gopkg/filetype/filetype.gopkg/stack/loader/registry.gopkg/stack/loader/yaml/loader.gopkg/function/exec_test.gopkg/stack/loader/hcl/loader.gopkg/stack/loader/json/loader.gopkg/stack/processor/processor.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: CLI command files must be thin wrappers that only handle CLI concerns (flags, arguments, command registration). All business logic must be delegated topkg/packages
Useflags.NewStandardParser()for command-specific flag parsing. NEVER callviper.BindEnv()orviper.BindPFlag()directly - use the unifiedpkg/flags/infrastructure instead. Register flags with Cobra command and bind to Viper for precedence handling
All CLI commands MUST implement the CommandProvider interface and register via command registry pattern incmd/internal/registry.go
Embed examples fromcmd/markdown/*_usage.mdusing//go:embed. Render withutils.PrintfMarkdown()
Files:
cmd/stack/convert.go
🧠 Learnings (62)
📓 Common learnings
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.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 **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.gopkg/function/exec_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : New configs support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`. Implement template functions in `internal/exec/template_funcs.go`, register them, add tests, and update documentation
Applied to files:
pkg/stack/processor/processor_test.gopkg/function/hcl.gopkg/filetype/filetype.gopkg/function/exec_test.gopkg/stack/processor/processor.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to pkg/**/*.go : Use interface-driven design: define interfaces for all major functionality, use dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen` with `//go:generate` directives, avoid integration tests by mocking external dependencies
Applied to files:
pkg/stack/processor/processor_test.gopkg/function/exec_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Applied to files:
pkg/stack/processor/processor_test.gopkg/stack/loader/json/loader_test.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 **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/stack/loader/json/loader_test.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:
errors/errors.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Use sentinel errors from `errors/errors.go` with ErrorBuilder for all user-facing errors. Use `errors.Is()` for checking errors, never string comparison. `WithCause()` preserves underlying error messages, `WithExplanation()` adds context, `WithHint()` provides guidance, `WithContext()` adds key-value pairs
Applied to files:
errors/errors.go
📚 Learning: 2025-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:
errors/errors.gopkg/filetype/filetype.gopkg/stack/loader/hcl/loader.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:
errors/errors.gopkg/stack/loader/hcl/loader.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
pkg/function/hcl.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/hcl.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/hcl.gopkg/filetype/filetype.gocmd/stack/convert.gopkg/stack/loader/hcl/loader.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
pkg/function/hcl.gopkg/filetype/filetype.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/function/hcl.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:
pkg/function/hcl.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
pkg/function/hcl.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
pkg/function/hcl.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/function/hcl.gopkg/filetype/filetype.gocmd/stack/convert.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/hcl.gopkg/filetype/filetype.gopkg/stack/loader/hcl/loader.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/hcl.gopkg/function/exec_test.gointernal/exec/yaml_func_resolution_context.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/function/hcl.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/hcl.gopkg/filetype/filetype.gopkg/stack/loader/hcl/loader.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/hcl.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/hcl.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
pkg/filetype/filetype.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : NEVER use `assert.Contains(err.Error(), ...)` for error checking. ALWAYS use `assert.ErrorIs()` instead. NEVER use string comparison: `err.Error() == "..."` or `strings.Contains(err.Error(), ...)`
Applied to files:
pkg/filetype/filetype.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to pkg/**/*.go : Implement registry pattern for extensibility and plugin-like architecture. Define interfaces for provider implementations, register implementations in registry, generate mocks with mockgen. Examples: Command Registry in `cmd/internal/registry.go`, Store Registry in `pkg/store/registry.go`
Applied to files:
pkg/stack/loader/registry.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/stack/loader/yaml/loader.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 cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/stack/convert.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 cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
cmd/stack/convert.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:
cmd/stack/convert.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to cmd/**/*.go : CLI command files must be thin wrappers that only handle CLI concerns (flags, arguments, command registration). All business logic must be delegated to `pkg/` packages
Applied to files:
cmd/stack/convert.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
cmd/stack/convert.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to cmd/**/*.go : All CLI commands MUST implement the CommandProvider interface and register via command registry pattern in `cmd/internal/registry.go`
Applied to files:
cmd/stack/convert.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 Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
cmd/stack/convert.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/stack/convert.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:
cmd/stack/convert.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:
cmd/stack/convert.gopkg/stack/loader/hcl/loader.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:
cmd/stack/convert.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
cmd/stack/convert.go
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
cmd/stack/convert.go
📚 Learning: 2024-12-13T15:28:13.630Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
Applied to files:
cmd/stack/convert.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:
cmd/stack/convert.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to pkg/**/*.go : Create focused purpose-built packages in `pkg/` for new functionality, each with clear responsibility. DO NOT add new functions to `pkg/utils/` or `internal/exec/`. Each package should have focused business logic with tests
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Before implementing new functionality, search `internal/exec/` and `pkg/` for existing code. Extend, don't duplicate
Applied to files:
pkg/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 **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to tests/**/*.go : Use `cmd.NewTestKit(t)` for any test touching RootCmd. This auto-cleans RootCmd state. Tests skip gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
pkg/function/exec_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Use dependency injection and interface-driven design to make code testable - avoid `os.Exit`, `CheckErrorPrintAndExit`, and direct external system calls in testable functions
Applied to files:
pkg/function/exec_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:
pkg/function/exec_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:
pkg/function/exec_test.go
📚 Learning: 2024-12-13T16:48:00.294Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:42-42
Timestamp: 2024-12-13T16:48:00.294Z
Learning: The function `withCacheFileLock` in `pkg/config/cache.go` is currently unused and left for future upgrade.
Applied to files:
pkg/stack/loader/hcl/loader.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/stack/loader/hcl/loader.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:
pkg/stack/loader/hcl/loader.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
pkg/stack/loader/hcl/loader.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:
pkg/stack/loader/hcl/loader.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
internal/exec/yaml_func_resolution_context.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/yaml_func_resolution_context.go
📚 Learning: 2025-10-08T06:48:07.499Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1602
File: internal/exec/stack_processor_utils.go:968-1003
Timestamp: 2025-10-08T06:48:07.499Z
Learning: The `FindComponentDependenciesLegacy` function in `internal/exec/stack_processor_utils.go` is legacy code that is not actively used and is kept only for backward compatibility purposes.
Applied to files:
internal/exec/yaml_func_resolution_context.go
📚 Learning: 2024-11-30T22:07:08.610Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.
Applied to files:
internal/exec/yaml_func_resolution_context.go
🧬 Code graph analysis (7)
pkg/stack/processor/processor_test.go (4)
pkg/function/phase.go (3)
Phase(6-6)PreMerge(11-11)PostMerge(15-15)pkg/stack/processor/processor.go (2)
New(26-33)Processor(20-23)errors/errors.go (1)
ErrNilProcessor(584-584)pkg/stack/processor/context.go (2)
NewStackContext(35-46)StackContext(10-32)
pkg/function/hcl.go (4)
pkg/stack/loader/registry.go (1)
Registry(16-20)pkg/function/function.go (1)
Function(11-28)pkg/perf/perf.go (1)
Track(121-138)pkg/function/defaults.go (1)
DefaultRegistry(10-26)
pkg/filetype/filetype.go (3)
pkg/function/defaults.go (1)
DefaultRegistry(10-26)pkg/function/hcl.go (1)
HCLEvalContextWithFunctions(39-45)pkg/utils/file_utils.go (1)
ErrFailedToProcessHclFile(17-17)
pkg/stack/loader/registry.go (4)
pkg/stack/loader/loader.go (1)
StackLoader(63-79)pkg/function/registry.go (2)
NewRegistry(18-25)Registry(11-15)pkg/perf/perf.go (1)
Track(121-138)errors/errors.go (2)
ErrDuplicateLoader(576-576)ErrLoaderNotFound(575-575)
cmd/stack/convert.go (4)
pkg/flags/parser.go (2)
GetString(117-126)GetBool(141-150)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/schema/schema.go (2)
ConfigAndStacksInfo(621-715)AtmosConfiguration(53-94)internal/exec/stack_convert.go (1)
ExecuteStackConvert(41-93)
internal/exec/yaml_func_resolution_context.go (2)
pkg/function/resolution/context.go (6)
Context(29-32)NewContext(45-52)GetOrCreateContext(86-98)ClearContext(101-106)ScopedContext(114-137)Node(14-25)errors/errors.go (1)
ErrCircularDependency(523-523)
pkg/stack/processor/processor.go (7)
pkg/stack/loader/registry.go (1)
Registry(16-20)pkg/perf/perf.go (1)
Track(121-138)errors/errors.go (2)
ErrNilProcessor(584-584)ErrProcessorNilContext(585-585)pkg/function/context.go (1)
NewExecutionContext(20-31)pkg/stack/processor/context.go (2)
NewStackContext(35-46)StackContext(10-32)pkg/function/phase.go (3)
PreMerge(11-11)PostMerge(15-15)Phase(6-6)pkg/function/errors.go (1)
ErrFunctionNotFound(8-8)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/stack_processor_utils.go (1)
628-655: Address inconsistent template file discovery for imports without extension.Lines 631-648 search for import files by iterating through
StackConfigExtensions()in order—which places template extensions (.yaml.tmpl,.yml.tmpl) after their non-template counterparts. This means an import likepath: mixins/regionwill findmixins/region.yamlfirst and never check formixins/region.yaml.tmpl.In contrast, lines 649-655 explicitly check for template versions when the import has a
.yaml/.ymlextension specified. This creates inconsistent behavior:
path: mixins/region→ loadsmixins/region.yaml(skips template)path: mixins/region.yaml→ loadsmixins/region.yaml.tmplif it existsConsider either reordering
StackConfigExtensions()to place template extensions first, or adding similar template-checking logic for the extensionless case.
🧹 Nitpick comments (2)
internal/exec/yaml_func_utils_context_test.go (1)
72-74: Optional: useassert.Lenfor clearer intent.
assert.Equal(t, 1, len(ctx.CallStack()))is correct, butassert.Len(t, ctx.CallStack(), 1)would more directly express the intent and produce a nicer failure message if this ever breaks.internal/exec/stack_processor_utils.go (1)
1193-1231: Well-structured extension-based parser routing.The function correctly handles:
- YAML family with position tracking
- JSON/HCL via the filetype package
- Performance tracking per guidelines
Two observations:
Unknown extension fallback (line 1229-1230): Silently falling back to YAML parsing for unknown extensions could mask configuration errors. If a user accidentally uses
.txtor a typo like.ymal, they'd get a cryptic YAML parse error rather than a clear "unsupported format" message.Type assertion safety (lines 1221-1224): The error message is clear, but consider whether
filetype.ParseByExtensioncould return a nested map type (e.g.,map[interface{}]interface{}) that would also fail this assertion.Consider making unknown extensions explicit:
- // Fallback to YAML parsing for unknown extensions. - return u.UnmarshalYAMLFromFileWithPositions[schema.AtmosSectionMapType](atmosConfig, content, filePath) + // Unknown extensions - could be an error, but fallback to YAML for backward compatibility. + log.Debug("Unknown file extension, falling back to YAML parsing", "file", filePath, "ext", ext) + return u.UnmarshalYAMLFromFileWithPositions[schema.AtmosSectionMapType](atmosConfig, content, filePath)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/root.go(1 hunks)errors/errors.go(1 hunks)internal/exec/stack_processor_utils.go(5 hunks)internal/exec/yaml_func_resolution_context_bench_test.go(1 hunks)internal/exec/yaml_func_utils_context_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- errors/errors.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
internal/exec/yaml_func_utils_context_test.gocmd/root.gointernal/exec/stack_processor_utils.gointernal/exec/yaml_func_resolution_context_bench_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Useassert.ErrorIs(t, err, ErrSentinel)for checking our own errors and stdlib errors. Only use string matching for third-party errors
NEVER useassert.Contains(err.Error(), ...)for error checking. ALWAYS useassert.ErrorIs()instead. NEVER use string comparison:err.Error() == "..."orstrings.Contains(err.Error(), ...)
Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoidos.Exit,CheckErrorPrintAndExit, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Usego.uber.org/mock/mockgenwith//go:generatedirectives for mock generation. Never create manual mocks. Pattern://go:generate go run go.uber.org/mock/mockgen@latest -source=filename.go -destination=mock_filename_test.go
Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Files:
internal/exec/yaml_func_utils_context_test.gointernal/exec/yaml_func_resolution_context_bench_test.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: CLI command files must be thin wrappers that only handle CLI concerns (flags, arguments, command registration). All business logic must be delegated topkg/packages
Useflags.NewStandardParser()for command-specific flag parsing. NEVER callviper.BindEnv()orviper.BindPFlag()directly - use the unifiedpkg/flags/infrastructure instead. Register flags with Cobra command and bind to Viper for precedence handling
All CLI commands MUST implement the CommandProvider interface and register via command registry pattern incmd/internal/registry.go
Embed examples fromcmd/markdown/*_usage.mdusing//go:embed. Render withutils.PrintfMarkdown()
Files:
cmd/root.go
🧠 Learnings (27)
📓 Common learnings
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/yaml_func_utils_context_test.gocmd/root.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/yaml_func_utils_context_test.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
internal/exec/yaml_func_utils_context_test.gocmd/root.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
internal/exec/yaml_func_utils_context_test.go
📚 Learning: 2025-10-08T06:48:07.499Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1602
File: internal/exec/stack_processor_utils.go:968-1003
Timestamp: 2025-10-08T06:48:07.499Z
Learning: The `FindComponentDependenciesLegacy` function in `internal/exec/stack_processor_utils.go` is legacy code that is not actively used and is kept only for backward compatibility purposes.
Applied to files:
internal/exec/yaml_func_utils_context_test.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/yaml_func_utils_context_test.gocmd/root.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
cmd/root.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
cmd/root.gointernal/exec/stack_processor_utils.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:
cmd/root.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:
cmd/root.go
📚 Learning: 2025-09-24T20:45:40.401Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: tests/fixtures/scenarios/atmos-auth/stacks/deploy/nonprod.yaml:3-4
Timestamp: 2025-09-24T20:45:40.401Z
Learning: In Atmos stack files, the correct syntax for importing other stack files is `import:` (singular), not `imports:` (plural). All stack files in the Atmos codebase consistently use `import:` followed by a list of paths to import.
Applied to files:
cmd/root.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/root.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
cmd/root.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:
cmd/root.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
cmd/root.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:
cmd/root.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to cmd/**/*.go : CLI command files must be thin wrappers that only handle CLI concerns (flags, arguments, command registration). All business logic must be delegated to `pkg/` packages
Applied to files:
cmd/root.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : New configs support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`. Implement template functions in `internal/exec/template_funcs.go`, register them, add tests, and update documentation
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2024-10-20T00:57:53.500Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 731
File: internal/exec/validate_stacks.go:0-0
Timestamp: 2024-10-20T00:57:53.500Z
Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
internal/exec/stack_processor_utils.go
🧬 Code graph analysis (2)
internal/exec/yaml_func_utils_context_test.go (1)
pkg/perf/perf.go (1)
CallStack(53-56)
internal/exec/stack_processor_utils.go (6)
pkg/utils/file_extensions.go (7)
StackConfigExtensions(22-31)YamlFileExtension(11-11)YmlFileExtension(12-12)YamlTemplateExtension(13-13)YmlTemplateExtension(14-14)JSONFileExtension(16-16)HCLFileExtension(17-17)pkg/schema/schema.go (2)
AtmosConfiguration(53-96)AtmosSectionMapType(14-14)pkg/utils/yaml_position.go (1)
PositionMap(14-14)pkg/utils/yaml_utils.go (1)
UnmarshalYAMLFromFileWithPositions(716-770)pkg/filetype/filetype_by_extension.go (1)
ParseByExtension(42-54)errors/errors.go (1)
ErrParseFile(18-18)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/root.go (1)
55-55: LGTM! Side-effect import follows the established pattern.The blank import correctly wires the stack command into the registry. The StackCommandProvider implementation in
cmd/stack/stack.goproperly implements the CommandProvider interface with all required methods and registers viainternal.Register()in the init() function. The convert subcommand is attached and configured correctly. The import is alphabetically sorted within the Atmos package group.internal/exec/yaml_func_resolution_context_bench_test.go (1)
60-60: Comment documenting removed benchmark looks good.The note about
BenchmarkGetGoroutineIDbeing removed andgetGoroutineIDmoving internal topkg/function/resolutionis clear, accurate, and consistent with the perf-related design decisions around goroutine ID handling. Based on learnings, this is a helpful breadcrumb for future readers rather than dead noise.internal/exec/yaml_func_utils_context_test.go (1)
237-243: CallStack-based isolation checks align with the new API and concurrency pattern.The updated checks using
ctx1.CallStack()/ctx2.CallStack()to verify a single frame and the expectedComponentper goroutine correctly adapt the test to the accessor-basedResolutionContextAPI while preserving the behavioral guarantee that contexts are goroutine-local. Using an error channel instead of touchingtesting.Tinside goroutines keeps the test concurrency-safe.Also applies to: 272-278
internal/exec/stack_processor_utils.go (2)
17-25: Import organization looks good.The new
filetypeimport is correctly placed in the Atmos packages group and alphabetically ordered.
443-466: Multi-format parsing integration looks solid.The switch to
parseStackConfigByExtensioncorrectly handles the three return values and preserves position tracking for YAML files. The provenance tracking at lines 462-466 appropriately guards against empty positions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/filetype/filetype.go (3)
19-19: Duplicate error definition persists.This error is also defined in
pkg/utils/file_utils.go. Per coding guidelines, sentinel errors should come fromerrors/errors.go. Having duplicates breakserrors.Is()comparisons.Consider consolidating to a single definition in the errors package, or importing from utils if that's the canonical location.
#!/bin/bash # Check for duplicate ErrFailedToProcessHclFile definitions rg -n "ErrFailedToProcessHclFile" --type=go
23-28: Minor comment inconsistency.Line 24 mentions
repo_root()butDefaultRegistryactually registersgit_root()per the relevant code snippet. Small doc fix keeps things clear.// hclEvalContext returns an HCL evaluation context with Atmos functions available. -// Functions like env(), exec(), template(), and repo_root() can be used in HCL expressions. +// Functions like env(), template(), and git_root() can be used in HCL expressions. +// Note: exec() is disabled (nil shellExecutor) for security during file parsing. func hclEvalContext() *hcl.EvalContext {
177-180: Extensive nolint directives warrant attention.Per coding guidelines: "NEVER add nolint for gocognit, cyclomatic complexity, magic numbers, or nestif - refactor instead." This line suppresses several of those.
Consider extracting helper functions:
processBlockWithLabels(current map[string]any, block *hclsyntax.Block, blockContent map[string]any)processBlockWithoutLabels(result map[string]any, block *hclsyntax.Block, blockContent map[string]any)This could reduce cognitive complexity enough to remove the linter suppressions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
internal/exec/yaml_func_resolution_context.go(4 hunks)pkg/filetype/filetype.go(3 hunks)pkg/function/hcl.go(1 hunks)pkg/function/phase.go(1 hunks)pkg/stack/loader/registry_test.go(1 hunks)pkg/utils/file_utils.go(0 hunks)tools/lintroller/rule_perf_track.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/utils/file_utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/stack/loader/registry_test.go
- pkg/function/hcl.go
- pkg/function/phase.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: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
pkg/filetype/filetype.gotools/lintroller/rule_perf_track.gointernal/exec/yaml_func_resolution_context.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Business logic packages must follow the Service-Oriented Architecture pattern with: one Service struct per domain, Provider interfaces for classes of problems, default implementations, mock implementations, and dependency injection for testability
Use interface-driven design: define interfaces for all major functionality, use dependency injection for testability, generate mocks withgo.uber.org/mock/mockgenwith//go:generatedirectives, avoid integration tests by mocking external dependencies
Create focused purpose-built packages inpkg/for new functionality, each with clear responsibility. DO NOT add new functions topkg/utils/orinternal/exec/. Each package should have focused business logic with tests
Implement registry pattern for extensibility and plugin-like architecture. Define interfaces for provider implementations, register implementations in registry, generate mocks with mockgen. Examples: Command Registry incmd/internal/registry.go, Store Registry inpkg/store/registry.go
Files:
pkg/filetype/filetype.go
🧠 Learnings (24)
📓 Common learnings
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : New configs support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`. Implement template functions in `internal/exec/template_funcs.go`, register them, add tests, and update documentation
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/filetype/filetype.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : New configs support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`. Implement template functions in `internal/exec/template_funcs.go`, register them, add tests, and update documentation
Applied to files:
pkg/filetype/filetype.gotools/lintroller/rule_perf_track.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
pkg/filetype/filetype.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/filetype/filetype.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/filetype/filetype.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:
pkg/filetype/filetype.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:
pkg/filetype/filetype.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/filetype/filetype.go
📚 Learning: 2024-10-23T20:13:23.054Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 731
File: pkg/utils/file_utils.go:198-202
Timestamp: 2024-10-23T20:13:23.054Z
Learning: In `pkg/utils/file_utils.go`, the current implementation of the `IsURL` function is considered sufficient; avoid suggesting more complex URL validation in future reviews.
Applied to files:
pkg/filetype/filetype.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/filetype/filetype.go
📚 Learning: 2024-10-31T07:09:31.983Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 737
File: internal/exec/vendor_utils.go:181-182
Timestamp: 2024-10-31T07:09:31.983Z
Learning: In `internal/exec/vendor_utils.go`, the variables `mergedSources` and `mergedImports` are declared and used later in the code. Do not suggest removing them as unused variables.
Applied to files:
pkg/filetype/filetype.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/filetype/filetype.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-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-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.gointernal/exec/yaml_func_resolution_context.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
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.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: 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:
tools/lintroller/rule_perf_track.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/yaml_func_resolution_context.go
📚 Learning: 2025-10-08T06:48:07.499Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1602
File: internal/exec/stack_processor_utils.go:968-1003
Timestamp: 2025-10-08T06:48:07.499Z
Learning: The `FindComponentDependenciesLegacy` function in `internal/exec/stack_processor_utils.go` is legacy code that is not actively used and is kept only for backward compatibility purposes.
Applied to files:
internal/exec/yaml_func_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:
internal/exec/yaml_func_resolution_context.go
📚 Learning: 2024-11-30T22:07:08.610Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.
Applied to files:
internal/exec/yaml_func_resolution_context.go
🧬 Code graph analysis (1)
pkg/filetype/filetype.go (2)
pkg/function/defaults.go (1)
DefaultRegistry(10-26)pkg/function/hcl.go (1)
HCLEvalContextWithFunctions(40-46)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (15)
tools/lintroller/rule_perf_track.go (1)
62-77: Phase receiver exclusion fits existing perf.Track exception patternAdding
Phasehere for a trivial enumString()in hot paths is in line with how this list already treats high-frequency or low-value receivers (e.g., spinner models, template funcs). The comment is clear and keeps the exception centralized in the linter config. No changes needed from my side.pkg/filetype/filetype.go (8)
3-17: Import organization looks good.Three groups properly separated: stdlib, 3rd-party, and Atmos packages. Well done.
50-57: Format detection approach is solid.Using
hclv1.Unmarshalfor quick format detection while actual parsing useshcl/v2makes sense. Keeps detection simple and lenient.
133-145: Clean delegation to parseHCLBody.Error handling is consistent with the new format constant. Good separation of concerns.
147-175: Two-path parsing approach is reasonable.The NOTE comment (lines 156-158) acknowledges the limitation around error assumption. The fallback to
parseHCLBodyWithBlocksprovides safety for genuine syntax errors. Documented trade-off is acceptable.
204-219: Block merging logic addresses past feedback.The else branch (lines 213-216) now handles type mismatch by overwriting. Good fix from previous review.
220-245: Labeled block handling is correctly guarded.Type assertions now include ok checks with proper error returns (lines 227-230, 238-241). Addresses the panic concern from previous review.
251-264: Fallback section handles edge cases appropriately.If the body isn't
*hclsyntax.Bodybut contains blocks, this will error out rather than silently dropping data. That's the right behavior.
266-297: Pre-existing ctyToGo function unchanged.This function wasn't modified in this PR. Two pre-existing observations for potential future improvement:
- Lines 272-274: The
if n, _ := ...; truepattern always returnsint64, even for floats.- Line 296: Returning raw
cty.Valuefor unhandled types may cause serialization issues downstream.Not blocking for this PR, but worth tracking.
internal/exec/yaml_func_resolution_context.go (6)
94-122: Well-structured error message.The error building logic correctly iterates through
ctx.inner.CallStack, usesparseNodeKeyto extract stack/component, and preserves the existing error format for backward compatibility. The chain of dependencies is clear.
124-132: Excellent fix for the parsing ambiguity!The new
parseNodeKeyuses"::"as delimiter, which resolves the past review concern about hyphenated names.SplitN(key, "::", 2)unambiguously splits"my-stack::vpc"into stack="my-stack", component="vpc".The fallback for keys without a delimiter is reasonable (treats the entire key as component).
147-167: CallStack accessor provides good backward compatibility.The conversion from
resolution.Nodeback toDependencyNodeis implemented correctly, with proper nil checks and use ofparseNodeKey. The comment on line 162 documents thatFunctionTypeis not preserved (already flagged in the earlier comment).
169-185: Perfect fix for the encapsulation issue!The
Visited()method now returns a copy of the internal map (lines 180-183), which addresses the past review concern about exposing internal state for mutation. This preserves encapsulation while maintaining backward compatibility.
62-73: Good fix on the delimiter ambiguity.The switch to
"::"as the delimiter addresses the past review concern about hyphenated names. The key format"stack::component"is now unambiguous andparseNodeKeycan reliably split it.Note:
FunctionTypeis intentionally not stored inresolution.Node(line 162). This is by design—the generic resolution context doesn't need function type information for cycle detection. Callers accessingCallStack()receiveDependencyNodeobjects whereFunctionTypeis set to an empty string, which is acceptable since no code depends on this field fromCallStack()results.
75-85: No issue found.The error handling in
Push()is correct. Theresolution.Context.Push()method can only return:
- A cycle detection error (via
buildCycleError())nilon successSince
inner.Push()has no other error paths, converting any error to a circular dependency error is appropriate and safe.
|
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 |
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cmd/workflow.go(1 hunks)internal/exec/stack_processor_process_stacks.go(2 hunks)internal/exec/workflow.go(1 hunks)internal/exec/workflow_utils.go(1 hunks)internal/exec/yaml_func_resolution_context_test.go(15 hunks)pkg/config/imports_error_paths_test.go(2 hunks)pkg/filetype/filetype.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/workflow.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/filetype/filetype.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
NEVER delete existing comments without a very strong reason. Comments document why/how/what/where. Preserve helpful comments, update to match code, refactor for clarity, add context when modifying.
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")()with a blank line to all public functions. Usenilif no atmosConfig parameter is available.
Define interfaces for all major functionality. Use dependency injection for testability. Generate mocks withgo.uber.org/mock/mockgen. Avoid integration tests by mocking external dependencies.
Use//go:generatedirectives withgo.uber.org/mock/mockgenfor mock generation. Never creat...
Files:
internal/exec/workflow_utils.gointernal/exec/workflow.gointernal/exec/stack_processor_process_stacks.gopkg/config/imports_error_paths_test.gointernal/exec/yaml_func_resolution_context_test.go
internal/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
For legacy patterns (internal/non-user-facing only): use
errors.Joinfor multiple errors,fmt.Errorf("%w", err)for chains (sequential call stack), orfmt.Errorfwith multiple%wlikeerrors.Join(Go 1.20+).
Files:
internal/exec/workflow_utils.gointernal/exec/workflow.gointernal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_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 and dependency injection for testability. Generate mocks withgo.uber.org/mock/mockgen. Use table-driven tests for comprehensive coverage. Target >80% coverage.
Test behavior, not implementation. Never test stub functions. Avoid tautological tests. Make code testable using DI. No coverage theater. Remove always-skipped tests. Table-driven tests need real scenarios. Useassert.ErrorIs(err, ErrSentinel)for our/stdlib errors.
Tests must call actual production code, never duplicate logic.
Uset.Skipf("reason")with clear context for test skipping. CLI tests auto-build temp binaries.
Never use pipe redirection (2>&1,| head,| tail) when running tests. Piping breaks TTY detection → ASCII fallback → wrong snapshots.
Tests skip gracefully with helpers fromtests/test_preconditions.go.
Files:
pkg/config/imports_error_paths_test.gointernal/exec/yaml_func_resolution_context_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: For complex domains with multiple operations, use Service-Oriented Architecture with one Service struct per domain, provider interfaces for classes of problems, default implementations, mock implementations, and dependency injection for testability.
Follow multi-provider registry pattern: 1) Define interface in dedicated package, 2) Implement per provider, 3) Register implementations, 4) Generate mocks.
Files:
pkg/config/imports_error_paths_test.go
🧠 Learnings (48)
📓 Common learnings
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
📚 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_utils.gointernal/exec/workflow.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_utils.gointernal/exec/workflow.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to internal/**/*.go : For legacy patterns (internal/non-user-facing only): use `errors.Join` for multiple errors, `fmt.Errorf("%w", err)` for chains (sequential call stack), or `fmt.Errorf` with multiple `%w` like `errors.Join` (Go 1.20+).
Applied to files:
internal/exec/workflow_utils.gointernal/exec/workflow.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
internal/exec/workflow_utils.gointernal/exec/workflow.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
internal/exec/workflow_utils.gointernal/exec/workflow.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
internal/exec/workflow_utils.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/workflow.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/workflow.gointernal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_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:
internal/exec/workflow.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:
internal/exec/workflow.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Search `internal/exec/` and `pkg/` before implementing. Extend, don't duplicate.
Applied to files:
internal/exec/workflow.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : Linux/macOS/Windows compatible. Use SDKs over binaries. Use `filepath.Join()`, not hardcoded path separators.
Applied to files:
internal/exec/workflow.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:
internal/exec/workflow.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_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:
internal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_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/stack_processor_process_stacks.go
📚 Learning: 2025-10-08T06:48:07.499Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1602
File: internal/exec/stack_processor_utils.go:968-1003
Timestamp: 2025-10-08T06:48:07.499Z
Learning: The `FindComponentDependenciesLegacy` function in `internal/exec/stack_processor_utils.go` is legacy code that is not actively used and is kept only for backward compatibility purposes.
Applied to files:
internal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2024-10-30T13:25:45.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:405-412
Timestamp: 2024-10-30T13:25:45.965Z
Learning: In `internal/exec/terraform_clean.go`, when appending `stackFolders` to `folders` in the `handleCleanSubCommand` function, it's unnecessary to check if `stackFolders` is nil before appending, because in Go, appending a nil slice is safe and does not cause a panic.
Applied to files:
internal/exec/stack_processor_process_stacks.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
internal/exec/stack_processor_process_stacks.gointernal/exec/yaml_func_resolution_context_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/stack_processor_process_stacks.go
📚 Learning: 2024-10-20T00:57:53.500Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 731
File: internal/exec/validate_stacks.go:0-0
Timestamp: 2024-10-20T00:57:53.500Z
Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
Applied to files:
internal/exec/stack_processor_process_stacks.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to internal/exec/template_funcs.go : Template functions: `atmos.Component/Stack/Setting()`, `terraform.output/state()`, `store.get()`, `exec()`, `env()`. Register in `internal/exec/template_funcs.go`
Applied to files:
internal/exec/stack_processor_process_stacks.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/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions. Avoid tautological tests. Make code testable using DI. No coverage theater. Remove always-skipped tests. Table-driven tests need real scenarios. Use `assert.ErrorIs(err, ErrSentinel)` for our/stdlib errors.
Applied to files:
pkg/config/imports_error_paths_test.gointernal/exec/yaml_func_resolution_context_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/config/imports_error_paths_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/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : Use functional options pattern for configuration instead of many parameters. Provides defaults and extensibility without breaking changes.
Applied to files:
pkg/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*_test.go : Tests skip gracefully with helpers from `tests/test_preconditions.go`.
Applied to files:
pkg/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : 80% minimum test coverage (CodeCov enforced). All features need tests. Use `make testacc-coverage` for coverage reports.
Applied to files:
pkg/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : Small focused files (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`.
Applied to files:
pkg/config/imports_error_paths_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*_test.go : Tests must call actual production code, never duplicate logic.
Applied to files:
pkg/config/imports_error_paths_test.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/imports_error_paths_test.go
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt 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.
Applied to files:
pkg/config/imports_error_paths_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/config/imports_error_paths_test.gointernal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
internal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
internal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
internal/exec/yaml_func_resolution_context_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:
internal/exec/yaml_func_resolution_context_test.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/exec/yaml_func_resolution_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:
internal/exec/yaml_func_resolution_context_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/yaml_func_resolution_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 call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
internal/exec/yaml_func_resolution_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 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:
internal/exec/yaml_func_resolution_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:
internal/exec/yaml_func_resolution_context_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:
internal/exec/yaml_func_resolution_context_test.go
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*_test.go : Use `t.Skipf("reason")` with clear context for test skipping. CLI tests auto-build temp binaries.
Applied to files:
internal/exec/yaml_func_resolution_context_test.go
🧬 Code graph analysis (4)
internal/exec/workflow_utils.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrInvalidWorkflowManifest(439-439)pkg/schema/schema.go (1)
Workflows(471-474)
internal/exec/workflow.go (2)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrRequiredFlagNotProvided(220-220)
internal/exec/stack_processor_process_stacks.go (2)
pkg/utils/file_extensions.go (1)
StackConfigExtensions(22-31)pkg/utils/file_utils.go (1)
TrimBasePathFromPath(93-97)
internal/exec/yaml_func_resolution_context_test.go (2)
pkg/perf/perf.go (1)
CallStack(53-56)internal/exec/yaml_func_resolution_context.go (4)
GetOrCreateResolutionContext(38-43)ClearResolutionContext(46-50)DependencyNode(15-20)NewResolutionContext(29-35)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (6)
internal/exec/yaml_func_resolution_context_test.go (3)
18-21: Consistent accessor pattern looks good.All tests correctly updated to use
CallStack()andVisited()accessor methods instead of direct field access. This aligns with the architecture change whereResolutionContextnow wraps an inner context.Also applies to: 39-41, 54-56, 61-64, 69-70, 216-217, 224-225, 245-248, 259-260, 277-277, 295-295, 318-318, 362-363, 388-390, 417-417, 500-500, 512-513, 522-523, 545-547, 555-558
561-602: Good test coverage for scoped context mechanism.The new test properly verifies that
scopedResolutionContext()creates an isolated context and correctly restores the original context state. Test structure is clean and assertions are thorough.
1-602: Excellent test coverage and quality.The test suite comprehensively covers resolution context functionality including cycle detection, error formatting, goroutine isolation, and edge cases. All tests follow best practices with proper error checking using
assert.ErrorIsfor sentinel errors and appropriate cleanup patterns.internal/exec/stack_processor_process_stacks.go (1)
54-54: No action needed. Template files are properly handled bytrimStackExtensions, which includesYamlTemplateExtensionandYmlTemplateExtensionin its extension matching. No template files currently exist in the codebase, and the function implementation correctly handles them if they do appear.internal/exec/workflow_utils.go (1)
245-248: Structured wrapping of invalid manifest errors looks good.Keeping
ErrInvalidWorkflowManifestas the sentinel and attaching the underlying parse error viaWithCause(err)(Lines 245-248) should makeerrors.Is(err, errUtils.ErrInvalidWorkflowManifest)reliable while preserving details.internal/exec/workflow.go (1)
60-64: Missing--fileerror should use consistent markdown formatting like other validation failures.Lines 61-63 return an ErrorBuilder error that won't be printed with markdown formatting, while other workflow validation errors (file not found, invalid manifest, etc.) use
CheckErrorPrintAndExitfor immediate formatted output. The error returned here isn't inKnownWorkflowErrors, so it bypasses the structured error handling incmd/workflow.goand gets printed as plain text by cobra instead. AdderrUtils.ErrRequiredFlagNotProvidedto theKnownWorkflowErrorsslice, or use theCheckErrorPrintAndExitpattern to align with established workflow error handling.⛔ Skipped due to learnings
Learnt from: samtholiya Repo: cloudposse/atmos PR: 959 File: cmd/describe_config.go:20-20 Timestamp: 2025-02-03T06:00:11.419Z Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.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.Learnt from: osterman Repo: cloudposse/atmos PR: 1599 File: tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden:0-0 Timestamp: 2025-10-11T19:12:38.832Z Learning: Usage Examples sections in error output are appropriate for command usage errors (incorrect syntax, missing arguments, invalid flags) but not for configuration validation errors (malformed workflow files, invalid settings in atmos.yaml). Configuration errors should focus on explaining what's wrong with the config, not command usage patterns.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.
- Fix extension matching bug in trimStackExtensions that incorrectly trimmed compound extensions (.yaml.tmpl) by matching base extensions (.yaml) first. Now sorts extensions by length descending before matching. - Add comprehensive unit tests for trimStackExtensions covering simple, compound, and edge case extensions. - Make TestGeneratePatterns_DirectoryPath order-independent by checking extension presence rather than index position. - Make TestGeneratePatterns_FileWithoutExtension order-independent using assert.Contains instead of assert.Equal on indexed positions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Introduces the new ability to define stacks in YAML, JSON, or HCL formats. The post covers: - Real challenges users face with YAML (implicit typing, whitespace sensitivity) - Benefits of each format without disparaging any - Practical examples showing the same config in all three formats - Guidance on when to use each format - HCL function support in the atmos:: namespace 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use correct labeled block syntax (`stack { components { terraform {
component "name" { } } } }`) instead of object assignment syntax.
🤖 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 ParseHCLStacks() for parsing HCL with labeled stack blocks - Add ParseYAMLStacks() for YAML multi-document support - Support multiple stacks per file in both formats - Add StackDocument type for multi-stack returns - Add comprehensive unit tests for HCL and YAML stack parsing - Add test fixtures for HCL labeled stacks and multi-stack files - Update blog post with HCL examples and multi-stack documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Announces the ability to define multiple stacks in a single YAML file using the standard multi-document format with `---` separators. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reverts modifications that incorrectly added multi-stack features to an already-released blog post about the `name` field feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Tabs component for side-by-side YAML/HCL comparison - Update title to be format-agnostic - Add tabbed "Get Started" section for each format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shows both `stack "name" { }` (explicit name) and `stack { }` (derived name)
variations in a tab group for easy comparison.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change /blog/ to /changelog/ to match the site's URL structure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add type-safe checks for map assertions in HCL block parsing to prevent panics - Use errors.Is(err, io.EOF) instead of string comparison for YAML parsing - Fix truncate marker in blog post to use HTML comment format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add 50+ go-cty stdlib functions to HCL evaluation context: - String: lower, upper, title, trim, replace, split, join, format, etc. - Collection: length, concat, contains, flatten, keys, values, merge, etc. - Numeric: abs, ceil, floor, max, min, pow, log, etc. - Encoding: jsonencode, jsondecode, csvdecode, base64encode/decode - Date/time: formatdate, timeadd - Regex: regex, regexall, can, try - Set: setintersection, setsubtract, setunion - Type: coalesce, tobool, tostring, tonumber, tolist, tomap, toset Register missing Atmos functions for HCL/JSON parity with YAML: - PreMerge: !include, !include.raw, !random - PostMerge placeholders: !terraform.output, !terraform.state, !store, !store.get, !aws.account_id, !aws.caller_identity_arn, !aws.caller_identity_user_id, !aws.region 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e labels
- Add multi-document YAML to multi-stack HCL conversion support
- Use stack name as HCL block label (stack "name" { } instead of stack { name = "name" })
- Fix HCL fixtures to use correct labeled component syntax (component "vpc" { })
- Move usage examples to embedded markdown file
- Refactor to reduce cyclomatic complexity by extracting helper functions
- Remove unused deprecated functions to stay under file length limit
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix function namespace from atmos. to atmos:: (HCL standard)
- Add support for multi-document YAML (--- separator)
- Add stack block wrapper with optional name label
- Add component labeled blocks (component "name" { })
- Use proper HCL block syntax (key { } not key = { })
- Add translations for include, random, and aws.* functions
- Update tests for new output format
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update intro to mention YAML, JSON, and HCL format support - Add Example section with single stack configuration using stack-example plugin - Add Multi-Stack Files section documenting multi-document YAML support - Show how multiple stacks can be defined in one file using --- separator 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix StackExample tab switching causing page scroll by using CSS grid overlay technique (all panels in same grid cell with visibility toggle) - Change Tabs from groupId to queryString to avoid anchor-based scrolling - Update multi-stack files section intro to be format-agnostic 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add PlaceholderFunction base type for PostMerge functions to reduce code duplication (store, terraform functions) - Consolidate store.go and terraform.go into placeholder_functions.go - Fix staticcheck QF1008: remove redundant embedded field from selector - Add new sentinel errors for stack loader operations - Enhance stack convert with multi-document support - Update remark-stack-example converter for HCL block syntax 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix security alert: Use proper regex escaping for all special characters in converter.js (fixes incomplete string escaping warning) - Document non-deterministic behavior of PreMerge functions in HCL parsing - Add justification comments for nolint directives in filetype.go (HCL parsing complexity requires deferred refactoring) - Remove redundant nolintlint directive 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add validation for rangeSize <= 0 after computing range - Compute result in int64 space before casting to int to prevent overflow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add missing error sentinels to errors/errors.go - Update ExecutionContext API in test files to use struct literals - Remove tests for non-existent functions/types after rebase - Update workflow tests to match new error behavior - Fix ProcessYAMLConfigFileWithContext return value count - Fix lint errors (errorlint, godot, staticcheck) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The HTTP client API changed to use functional options pattern. Updated fetchURL to use atmoshttp.WithTimeout() instead of passing timeout directly to NewDefaultClient. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused shell import (was used by old inline workflow execution) - Remove TestPrepareStepEnvironment_* tests that reference the now-removed prepareStepEnvironment function The workflow execution has been refactored to use executor.Execute() from the pkg/workflow package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove duplicate backend provisioning error declarations that were introduced during conflict resolution. Keep the consolidated declarations in the Backend provisioning errors section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove duplicate error declarations in errors/errors.go - Remove duplicate ErrBackendTypeRequired declaration - Fix ExecutionContext struct with duplicate fields in pkg/function/context.go - Remove placeholder_functions.go (duplicates store.go and terraform.go) - Restore tags.go from main branch for tag constants - Remove duplicate TagLiteral declaration from literal.go - Add filePermissions constant to terraform_generate_backend.go - Fix formatting in store.go (gofumpt) - Add perf.Track() to logBackend methods in pkg/yq/yq.go Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add IsExperimental method to StackCommandProvider - Add terminal import and convertToTermenvProfile function to cmd/root.go Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix duplicate cycle error text in resolution/context.go - Add trailing content check to JSON loader - Fix workflow steps nil check before accessing length - Add validation for nil/empty loaders in registry - Fix normalizeExtension edge case for empty string - Fix map mutation in stack_convert.go by cloning maps - Add missing id field to MDX frontmatter in usage.mdx - Add perf.Track to public functions in filetype.go and context.go Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Release Documentation RequiredThis PR is labeled
|
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
what
atmos stack convertcommand for converting stack configurations between YAML, JSON, and HCL formatscomponent "name") to prepare for future stack naming syntaxwhy
references
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.