feat(stack): Add multi-format stack support with convert command #1842
Conversation
Introduce extensible infrastructure for multi-format configuration handling: - pkg/function/resolution/: Generic cycle detection extracted from internal/exec - Goroutine-scoped context for safe concurrent resolution - Generalized Node type for any dependency tracking - pkg/function/: Function interface and registry - Phase-based execution (PreMerge/PostMerge) - Thread-safe registry with alias support - ExecutionContext for function runtime environment - pkg/stack/loader/: StackLoader interface and registry - Extension-based loader lookup - Functional options for load/encode configuration - Thread-safe registry with extension normalization - Refactored internal/exec to use pkg/function/resolution as thin wrapper - Added comprehensive tests achieving >97% coverage on all new packages This is Phase 1 of the extensible file handler registry implementation, preparing the foundation for JSON, HCL, and future format support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implement Phase 2 of the extensible file handler registry: Phase 2.1 - Simple function implementations in pkg/function/: - EnvFunction (!env): Environment variable access with defaults - ExecFunction (!exec): Shell command execution with JSON parsing - GitRootFunction (!repo-root): Git repository root discovery - TemplateFunction (!template): JSON template parsing in YAML Phase 2.2 - Deferred complex functions (store, terraform) to Phase 5 due to deep dependencies on AtmosConfiguration and stack context. Phase 2.3 - YAML loader in pkg/stack/loader/yaml/: - Thread-safe content-aware caching with SHA256 hashing - Double-checked locking for concurrent access - Position extraction for provenance tracking - Panic recovery for unsupported types in encoder All implementations follow interface-driven design with dependency injection for testability. Test coverage: function 94.7%, resolution 97.2%, loader 98.5%, yaml 93.8%. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implement pkg/stack/loader/json/ package for JSON configuration support: - JSONLoader implements StackLoader interface - Thread-safe content-aware caching with SHA256 hashing - Double-checked locking for concurrent access - Position extraction for provenance tracking - Number precision preservation using json.Number - Compact output mode with WithCompactOutput option - HTML escape disabled for cleaner URLs in output Test coverage includes: - Basic and nested object parsing - Array and mixed type handling - Caching behavior and concurrent access - Position metadata extraction - Encode options (indent, compact) - Special character and large number handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add HCL loader package implementing StackLoader interface for parsing and encoding HashiCorp Configuration Language (HCL) files. Features: - Load/LoadWithMetadata for parsing .hcl and .tf files - Encode for generating HCL output from Go data structures - ParseBlocks for Terraform-style block parsing - Content-aware caching with SHA256 hashing - Thread-safe with double-checked locking pattern - Position extraction for provenance tracking - Full cty.Value to Go type conversion and vice versa Technical details: - Uses hashicorp/hcl/v2 and go-cty libraries - Handles all HCL types: strings, numbers, bools, lists, objects, null - Preserves number precision with big.Float - Refactored for linter compliance (static errors, reduced complexity) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add stack processor package that orchestrates function execution with stack-specific context, bridging the generic function package with stack processing needs. New files: - pkg/stack/processor/processor.go - Main Processor struct - pkg/stack/processor/context.go - StackContext with stack-specific fields - pkg/stack/processor/errors.go - Static error definitions - pkg/stack/processor/processor_test.go - Comprehensive tests - pkg/stack/processor/context_test.go - Context tests Features: - ProcessPreMerge for initial file loading (pre-merge functions) - ProcessPostMerge for merged configuration (post-merge functions) - StackContext with stack, component, skip list, dry run support - Function detection and phase-based execution - Builder pattern for StackContext with method chaining - Context cloning for safe modifications Technical details: - Integrates with pkg/function/Registry for function lookup - Integrates with pkg/stack/loader/Registry for format handling - Recursive data processing for maps, slices, and strings - Context cancellation support - Skip list for selective function execution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add unified defaults and factory functions for creating fully configured stack processors with default loaders and functions. New files: - pkg/stack/defaults.go - DefaultLoaderRegistry, DefaultProcessor, IsExtensionSupported - pkg/stack/defaults_test.go - Tests for default factories - pkg/stack/loader/defaults.go - SupportedExtensions helper - pkg/stack/loader/defaults_test.go - Tests for loader defaults Features: - DefaultLoaderRegistry() creates registry with YAML, JSON, HCL loaders - DefaultProcessor() creates processor with all default functions and loaders - IsExtensionSupported() for quick extension validation - SupportedExtensions() returns list of all supported extensions Integration points: - Factory functions in pkg/stack/ avoid import cycles - Provides clean API for consumers to get fully configured stack processing - Shell executor injection for !exec function support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add remark-stack-example plugin and StackExample component for displaying
configuration in YAML, JSON, and HCL formats with automatic function syntax
translation.
Components:
- website/plugins/remark-stack-example/ - Remark plugin that transforms
```yaml stack-example code blocks into tabbed multi-format views
- website/src/components/StackExample/ - React component with tabs UI
Features:
- Automatic YAML to JSON/HCL conversion
- Function syntax translation between formats:
- YAML: !env VAR → JSON: ${env:VAR} → HCL: atmos_env("VAR")
- !exec, !template, !repo-root, !terraform.output, !store supported
- Integrates with Docusaurus tabs for format selection
Usage:
```yaml stack-example
settings:
region: !env AWS_REGION
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Support title= in code block meta string for stack-example blocks. Now supports: ```yaml title="atmos.yaml" stack-example 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add support for converting Atmos stack configurations between YAML, JSON, and HCL formats with an `atmos stack convert` command. All formats parse to the same internal data structure, enabling seamless bidirectional conversion. HCL format uses labeled blocks (component "name") to prepare for future stack naming syntax. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning This PR is blocked from merging because a required semver label is missing. You'll need to add one before this PR can be merged. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
📝 WalkthroughWalkthroughThis PR introduces multi-format stack configuration support to Atmos, enabling parsing, processing, and conversion of YAML, JSON, and HCL stack files. It adds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 }
| // Validate required flag. | ||
| if toFormat == "" { | ||
| _ = ui.Error("--to flag is required (yaml, json, or hcl)") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant validation.
The --to flag is already marked as required on line 66, so Cobra enforces this check. This manual validation is redundant and has incorrect error handling (returns nil after an error, allowing the command to exit successfully).
Apply this diff to remove the redundant check:
RunE: func(cmd *cobra.Command, args []string) error {
// Get flag values.
toFormat, _ := cmd.Flags().GetString("to")
outputPath, _ := cmd.Flags().GetString("output")
dryRun, _ := cmd.Flags().GetBool("dry-run")
- // Validate required flag.
- if toFormat == "" {
- _ = ui.Error("--to flag is required (yaml, json, or hcl)")
- return nil
- }
-
// Load atmos configuration.
atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)🤖 Prompt for AI Agents
In cmd/stack/convert.go around lines 43 to 47, remove the redundant manual
validation block that checks if toFormat == "" and logs ui.Error then returns
nil; Cobra already enforces the required --to flag (declared at line 66), and
the current code incorrectly returns nil after reporting an error. Delete the
entire if-block so command flow relies on Cobra's required flag enforcement and
no erroneous nil return remains.
| // toResolutionNode converts a DependencyNode to a resolution.Node. | ||
| func (node DependencyNode) toResolutionNode() resolution.Node { | ||
| return resolution.Node{ | ||
| Key: fmt.Sprintf("%s-%s", node.Stack, node.Component), | ||
| Label: fmt.Sprintf("Component '%s' in stack '%s'", node.Component, node.Stack), | ||
| CallInfo: node.FunctionCall, | ||
| } | ||
| } |
There was a problem hiding this comment.
Key format may be ambiguous with hyphenated names.
Using stack-component format with hyphen separator breaks if either stack or component names contain hyphens. For example, my-stack + vpc → my-stack-vpc parses as stack=my, component=stack-vpc.
Consider using a delimiter unlikely to appear in names (e.g., \x00 or ::) or a structured format.
🤖 Prompt for AI Agents
internal/exec/yaml_func_resolution_context.go lines 62-69: the Key is built as
fmt.Sprintf("%s-%s", node.Stack, node.Component) which is ambiguous when
stack/component names contain hyphens; change Key to a delimiter or structured
format that cannot collide with names (for example use a null byte or "::" like
fmt.Sprintf("%s\x00%s", node.Stack, node.Component) or JSON encoding) and update
any code that parses/consumes this Key to split/deserialize using the new
delimiter/format so stack and component are reconstructed unambiguously.
| // parseNodeKey extracts stack and component from a key in format "stack-component". | ||
| func parseNodeKey(key string) (component, stack string) { | ||
| parts := strings.SplitN(key, "-", 2) | ||
| if len(parts) == 2 { | ||
| return parts[1], parts[0] | ||
| } | ||
| return key, "" | ||
| } |
There was a problem hiding this comment.
parseNodeKey is fragile with hyphenated names.
This confirms the ambiguity issue. SplitN(key, "-", 2) treats my-stack-vpc as stack=my, component=stack-vpc when it might be stack=my-stack, component=vpc.
🤖 Prompt for AI Agents
In internal/exec/yaml_func_resolution_context.go around lines 120 to 127,
parseNodeKey currently splits on the first hyphen which misparses names like
"my-stack-vpc"; change it to split on the last hyphen so the component is the
suffix after the final '-' and the stack is everything before it. Implement this
by finding the last index of '-' (strings.LastIndex), if found return key[:idx]
as stack and key[idx+1:] as component; if not found return key as component and
empty stack; ensure edge-cases (leading/trailing hyphen) produce sensible
non-panicking results.
| if existing, ok := result[block.Type]; ok { | ||
| // If the key already exists, merge the content. | ||
| if existingMap, ok := existing.(map[string]any); ok { | ||
| for k, v := range blockContent { | ||
| existingMap[k] = v | ||
| } | ||
| } | ||
| } else { | ||
| result[block.Type] = blockContent | ||
| } |
There was a problem hiding this comment.
Block merging silently overwrites on type mismatch.
When existing is not a map[string]any, the merge is skipped silently. This could hide configuration errors.
Consider logging a warning when type mismatch prevents merging:
if existingMap, ok := existing.(map[string]any); ok {
for k, v := range blockContent {
existingMap[k] = v
}
+ } else {
+ // Type mismatch - existing value is not a map, overwrite
+ result[block.Type] = blockContent
}🤖 Prompt for AI Agents
In pkg/filetype/filetype.go around lines 204 to 213, the merge silently skips
when the existing value under result[block.Type] is not a map[string]any,
potentially hiding configuration errors. Add a warning log in the branch where
the type assertion to map[string]any fails, indicating a type mismatch and that
the merge is skipped. This will help surface issues when merging is not possible
due to unexpected data types.
| typeMap := current[block.Type].(map[string]any) | ||
| for i, label := range block.Labels { | ||
| if i == len(block.Labels)-1 { | ||
| typeMap[label] = blockContent | ||
| } else { | ||
| if _, ok := typeMap[label]; !ok { | ||
| typeMap[label] = make(map[string]any) | ||
| } | ||
| typeMap = typeMap[label].(map[string]any) | ||
| } | ||
| } |
There was a problem hiding this comment.
Type assertion without ok check could panic.
Line 221 and 229 use unchecked type assertions. If the value is not map[string]any, this will panic.
- typeMap := current[block.Type].(map[string]any)
+ typeMap, ok := current[block.Type].(map[string]any)
+ if !ok {
+ return nil, fmt.Errorf("%w, file: %s, block type %s has unexpected value type", ErrFailedToProcessHclFile, filename, block.Type)
+ }
for i, label := range block.Labels {
if i == len(block.Labels)-1 {
typeMap[label] = blockContent
} else {
if _, ok := typeMap[label]; !ok {
typeMap[label] = make(map[string]any)
}
- typeMap = typeMap[label].(map[string]any)
+ labelMap, ok := typeMap[label].(map[string]any)
+ if !ok {
+ return nil, fmt.Errorf("%w, file: %s, block label %s has unexpected value type", ErrFailedToProcessHclFile, filename, label)
+ }
+ typeMap = labelMap
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typeMap := current[block.Type].(map[string]any) | |
| for i, label := range block.Labels { | |
| if i == len(block.Labels)-1 { | |
| typeMap[label] = blockContent | |
| } else { | |
| if _, ok := typeMap[label]; !ok { | |
| typeMap[label] = make(map[string]any) | |
| } | |
| typeMap = typeMap[label].(map[string]any) | |
| } | |
| } | |
| typeMap, ok := current[block.Type].(map[string]any) | |
| if !ok { | |
| return nil, fmt.Errorf("%w, file: %s, block type %s has unexpected value type", ErrFailedToProcessHclFile, filename, block.Type) | |
| } | |
| for i, label := range block.Labels { | |
| if i == len(block.Labels)-1 { | |
| typeMap[label] = blockContent | |
| } else { | |
| if _, ok := typeMap[label]; !ok { | |
| typeMap[label] = make(map[string]any) | |
| } | |
| labelMap, ok := typeMap[label].(map[string]any) | |
| if !ok { | |
| return nil, fmt.Errorf("%w, file: %s, block label %s has unexpected value type", ErrFailedToProcessHclFile, filename, label) | |
| } | |
| typeMap = labelMap | |
| } | |
| } |
| result := make(map[string]any) | ||
| for name, attr := range content.Attributes { | ||
| ctyValue, attrDiags := attr.Expr.Value(nil) | ||
| if attrDiags != nil && attrDiags.HasErrors() { | ||
| continue | ||
| } | ||
| positions := make(map[string]loader.Position) | ||
| goValue, err := ctyToGo(ctyValue, name, positions) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| result[name] = goValue |
There was a problem hiding this comment.
Errors silently swallowed during partial content extraction.
When attribute evaluation fails, the error is discarded via continue. This could hide legitimate parse issues. Consider logging at debug level or collecting errors.
🤖 Prompt for AI Agents
In pkg/stack/loader/hcl/loader.go around lines 510 to 521, attribute evaluation
and ctyToGo conversion errors are currently ignored with bare continues; instead
capture and surface them: when attrDiags has errors, append those diagnostics
(including attribute name and source position) to a collector slice or log them
at debug level rather than silently continuing, and do the same for the err
returned by ctyToGo (include error text, attribute name and position); ensure
the function either returns or attaches these collected errors/diagnostics to
the overall result so callers can inspect partial failures while still allowing
partial extraction to proceed.
| // findKeyPosition finds the approximate line and column of a key in JSON content. | ||
| func (l *Loader) findKeyPosition(content []byte, key string) loader.Position { | ||
| // Search for the quoted key followed by colon. | ||
| searchPattern := fmt.Sprintf(`"%s"`, key) | ||
| idx := bytes.Index(content, []byte(searchPattern)) | ||
| if idx == -1 { | ||
| return loader.Position{Line: 1, Column: 1} | ||
| } | ||
|
|
||
| // Count lines and find column. | ||
| line := 1 | ||
| column := 1 | ||
| for i := 0; i < idx; i++ { | ||
| if content[i] == '\n' { | ||
| line++ | ||
| column = 1 | ||
| } else { | ||
| column++ | ||
| } | ||
| } | ||
|
|
||
| return loader.Position{Line: line, Column: column} | ||
| } |
There was a problem hiding this comment.
Position tracking finds first key occurrence only.
findKeyPosition searches for the first match of "key" in content. For nested structures with repeated key names (e.g., "name" at multiple levels), this returns the first occurrence, not necessarily the correct one.
If accurate positions matter for error reporting, consider tracking byte offset during parsing or using a streaming approach. For basic provenance, this approximation may suffice.
| func TestRegistryConcurrentAccess(t *testing.T) { | ||
| r := NewRegistry() | ||
| done := make(chan bool) | ||
|
|
||
| // Concurrent registration. | ||
| go func() { | ||
| for i := 0; i < 100; i++ { | ||
| loader := newMockLoader("YAML", []string{".yaml"}) | ||
| _ = r.Register(loader) | ||
| _ = r.Unregister("YAML") | ||
| } | ||
| done <- true | ||
| }() | ||
|
|
||
| // Concurrent reads. | ||
| go func() { | ||
| for i := 0; i < 100; i++ { | ||
| _ = r.HasExtension(".yaml") | ||
| _, _ = r.GetByExtension(".yaml") | ||
| _ = r.List() | ||
| } | ||
| done <- true | ||
| }() | ||
|
|
||
| <-done | ||
| <-done | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Makefile" -o -path ".github/workflows/*" -type f | head -20Repository: cloudposse/atmos
Length of output: 186
🏁 Script executed:
if [ -f Makefile ]; then cat -n Makefile | grep -i -C 3 "test\|race"; fiRepository: cloudposse/atmos
Length of output: 3227
🏁 Script executed:
if [ -d .github/workflows ]; then find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs cat -n | grep -i -C 3 "test\|race"; fiRepository: cloudposse/atmos
Length of output: 14822
🏁 Script executed:
rg -n 'TESTARGS.*race|race.*TESTARGS' --type yaml --type shell .github/workflows/ scripts/ 2>/dev/null || echo "No dynamic TESTARGS with race found"Repository: cloudposse/atmos
Length of output: 95
🏁 Script executed:
rg -n '\-race' . --type go --type yaml --type make --type shell 2>/dev/null | head -30Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Check the actual acceptance test command more carefully
grep -r "go test" . --include="Makefile" --include="*.sh" | grep -v ".git"Repository: cloudposse/atmos
Length of output: 350
Race detection not currently enabled in CI—add -race flag to test targets.
The test exercises concurrent registration/unregistration with reads, which is good. However, race detection is not currently enabled in CI. The testacc target and other test commands in the Makefile do not include the -race flag. Consider adding race detection to at least the primary test target:
go test -race $(TEST) $(TESTARGS) -timeout 40mThis will catch subtle data races that the current iteration count (100) alone might miss.
🤖 Prompt for AI Agents
In pkg/stack/loader/registry_test.go around lines 239 to 265, the reviewer notes
CI does not run tests with the Go race detector; update the repo's Makefile test
targets (at least the primary test and testacc targets) to include the -race
flag when invoking go test (e.g., change go test $(TEST) $(TESTARGS) ... to go
test -race $(TEST) $(TESTARGS) ...), and ensure CI uses those updated targets so
the concurrent test runs under race detection.
| package processor | ||
|
|
||
| import "errors" | ||
|
|
||
| // Static errors for processor operations. | ||
| var ( | ||
| // ErrProcessingFailed is returned when processing fails. | ||
| ErrProcessingFailed = errors.New("processing failed") | ||
|
|
||
| // ErrFunctionNotFound is returned when a function is not found. | ||
| ErrFunctionNotFound = errors.New("function not found") | ||
|
|
||
| // ErrInvalidData is returned when the data structure is invalid. | ||
| ErrInvalidData = errors.New("invalid data structure") | ||
|
|
||
| // ErrSkippedFunction is returned when a function is skipped. | ||
| ErrSkippedFunction = errors.New("function skipped") | ||
|
|
||
| // ErrCycleDetected is returned when a dependency cycle is detected. | ||
| ErrCycleDetected = errors.New("dependency cycle detected") | ||
|
|
||
| // ErrNilProcessor is returned when a nil processor is used. | ||
| ErrNilProcessor = errors.New("nil processor") | ||
|
|
||
| // ErrNilContext is returned when a nil context is provided. | ||
| ErrNilContext = errors.New("nil context") | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine errors/errors.go
echo "=== Looking for errors/errors.go ==="
fd -t f "errors\.go" | grep -E "errors/errors\.go|errors\.go$" | head -5
echo ""
echo "=== Checking pkg/stack/processor directory ==="
fd -t f "\.go$" pkg/stack/processor/ | head -10
echo ""
echo "=== Examining pkg/stack/processor/errors.go imports and potential cycle ==="
if [ -f "pkg/stack/processor/errors.go" ]; then
echo "File exists at: pkg/stack/processor/errors.go"
head -10 pkg/stack/processor/errors.go
fi
echo ""
echo "=== Finding errors package location ==="
fd -t d "errors" --max-depth 2
echo ""
echo "=== Checking if errors/errors.go exists and its content ==="
if [ -f "errors/errors.go" ]; then
echo "=== errors/errors.go found ==="
head -50 errors/errors.go
else
echo "errors/errors.go not found at root level"
find . -name "errors.go" -type f | grep -v node_modules | head -10
fiRepository: cloudposse/atmos
Length of output: 4410
🏁 Script executed:
#!/bin/bash
echo "=== Checking processor.go imports ==="
head -30 pkg/stack/processor/processor.go
echo ""
echo "=== Checking what imports pkg/stack/processor ==="
rg "pkg/stack/processor" --type go -l | head -10
echo ""
echo "=== Checking other local error patterns in codebase ==="
echo "=== pkg/function/errors.go ==="
head -20 pkg/function/errors.go
echo ""
echo "=== pkg/stack/loader/errors.go ==="
head -20 pkg/stack/loader/errors.go
echo ""
echo "=== Checking if errors/errors.go imports any processor or stack packages ==="
grep -n "processor\|stack" errors/errors.go || echo "No processor/stack imports in errors/errors.go"Repository: cloudposse/atmos
Length of output: 4977
Move processor-specific errors to errors/errors.go to follow centralized error definitions pattern.
These static errors should be centralized in errors/errors.go per coding guidelines. Unlike the profiler package (which has a documented import cycle justifying local errors), the processor package has no import cycle preventing centralization. While pkg/function/errors.go and pkg/stack/loader/errors.go also define local errors, maintaining centralization improves consistency and error discoverability across the codebase.
| <dt>`reverse(str)`</dt> | ||
| <dd>Reverse a string. `reverse("hello")` → `"olleh"`</dd> |
There was a problem hiding this comment.
Incorrect function description.
The reverse() function in Terraform/OpenTofu reverses a list or tuple, not a string. The current documentation states it reverses a string.
Apply this diff to correct the description:
- <dt>`reverse(str)`</dt>
- <dd>Reverse a string. `reverse("hello")` → `"olleh"`</dd>
+ <dt>`reverse(list)`</dt>
+ <dd>Reverse a list. `reverse(["a", "b", "c"])` → `["c", "b", "a"]`</dd>🤖 Prompt for AI Agents
In website/docs/functions/hcl/stdlib.mdx around lines 62 to 63, the
documentation incorrectly states that reverse(str) reverses a string; update the
description to state that reverse() reverses a list or tuple (not a string) and
replace the example with a list/tuple example (e.g., reverse(["a","b","c"]) →
["c","b","a"] or similar) so the text and example reflect the correct
Terraform/OpenTofu behavior.
|
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 |
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
Release Notes
New Features
stack convertcommand to convert stack configurations between YAML, JSON, and HCL formats with--dry-runand file output support.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.