feat: Implement configurable output for list commands#1798
Conversation
…ipeline - Implement reusable renderer pipeline (filter, sort, column, format, output) - Create 5 pkg/list packages with >88% test coverage each - Add extraction functions for components, stacks, vendor - Update Priority Tier 1 commands: components (DEV-2805), stacks, vendor (DEV-2806) - Add schema support for List config in Stacks and Components - Create 17 standardized flag wrappers using With* naming convention - Enable universal filtering, sorting, column selection, and format options 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughUnifies and rewrites the "list" subsystem into a render pipeline (extract → filter → select columns → sort → format → output), adds configurable templated columns, tri-state boolean filters, provenance-based import-tree resolution/tree format, flag-wrapper parser with dynamic --columns completion, caching/provenance plumbing, many tests, and docs/examples. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
Migrate workflows, values, and instances commands to use the new renderer pipeline and flag wrapper pattern for consistent flag handling. ## Changes ### cmd/list/workflows.go - Migrate to full renderer pipeline (filter → column → sort → format) - Replace custom flag parsing with NewListParser flag wrappers - Add buildWorkflowFilters(), getWorkflowColumns(), buildWorkflowSorters() - Update return type from string to error - Add support for --columns and --sort flags - Maintain backward compatibility with --file filter - Use static errors from errors/errors.go ### pkg/list/extract_workflows.go (NEW) - Create ExtractWorkflows() to transform workflow manifests into []map[string]any - Extract workflow data with file, workflow name, description, and step count - Support file filtering via --file flag - Handle workflow directory traversal and YAML parsing - Use static errors (ErrParseFile, ErrWorkflowDirectoryDoesNotExist) ### pkg/list/extract_workflows_test.go (NEW) - Add comprehensive tests for workflow extraction - Test empty workflows, single workflow, multiple workflows - Test nil workflows handling ### cmd/list/values.go - Replace newCommonListParser with NewListParser using flag wrappers - Use WithFormatFlag, WithDelimiterFlag, WithStackFlag, WithQueryFlag - Use WithMaxColumnsFlag, WithAbstractFlag, WithProcessTemplatesFlag - Use WithProcessFunctionsFlag - Maintain existing FilterAndListValues logic (already using reusables) - Update both valuesCmd and varsCmd parsers ### cmd/list/instances.go - Replace newCommonListParser with NewListParser using flag wrappers - Use WithFormatFlag, WithDelimiterFlag, WithMaxColumnsFlag - Use WithStackFlag, WithQueryFlag, WithUploadFlag - Maintain existing ExecuteListInstancesCmd logic ## Testing All tests pass: - go test ./cmd/list/... ./pkg/list/... ✓ - All flag wrapper tests pass ✓ - All extraction tests pass ✓ ## Impact - Workflows command now supports universal --columns and --sort flags - Values and instances commands use consistent flag wrapper pattern - All Tier 2 commands follow same architectural pattern as Tier 1 - Maintains backward compatibility with existing flags 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix critical bug where empty format string caused "unsupported format:" error when running list commands without explicit --format flag. ## Changes ### pkg/list/renderer/renderer.go - Add default handling in formatTable() to use table format when empty - Prevents "unsupported format:" error on commands like `atmos list stacks` ### pkg/list/renderer/renderer_integration_test.go (NEW) - Add comprehensive integration tests for all format types - Test empty format string (should default to table) - Test invalid format string (should error) - Test rendering with sorting - Test rendering with empty data - All tests initialize I/O context properly ## Testing - All renderer tests pass (91.8% coverage) - Fixed user-reported issue: `atmos list stacks` now works - Test all formats: table, json, yaml, csv, tsv, empty ## Impact - Fixes critical regression where basic list commands failed - Empty format now correctly defaults to table format - Maintains backward compatibility 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace thick bordered table with clean header-only underline - Use simple local styles (bold header, plain cells) like version list - Change border to gray (color 8) for consistency - Remove unused theme import This gives list commands the same polished look as atmos version list.
…support - Add ExtractInstances() to convert schema.Instance to renderer format - Add getInstanceColumns() to read column config from atmos.yaml - Update ExecuteListInstancesCmd to use renderer pipeline - Remove obsolete formatInstances() function and related tests - Add custom column configuration to quick-start-advanced example - Support all output formats (table, json, yaml, csv, tsv) Custom columns configured in atmos.yaml (components.list.columns): - Component, Stack, Tenant, Environment, Stage, Region All existing list instances tests pass with renderer migration.
- Move Component column to the end for better readability - Add Description column from metadata.description field - Add description metadata to vpc and vpc-flow-logs-bucket components Column order: Stack → Tenant → Environment → Stage → Region → Description → Component This makes the output more readable by grouping organizational fields (tenant, environment, stage, region) before the descriptive and component fields.
…data to renderer pipeline This change fixes an anti-pattern where list commands were using log.Info() for user-facing messages instead of proper UI output methods. It also migrates the list metadata command to use the renderer pipeline for consistent table output with customizable columns. ## Changes ### Anti-pattern fixes - **cmd/list/settings.go**: Replace `log.Info()` with `ui.Info()` for "No settings found" messages - **cmd/list/values.go**: Replace `log.Info()` with `ui.Info()` for "No vars/values found" messages - **pkg/list/list_metadata.go**: Remove `log.Error()` calls; errors are now properly returned - **cmd/list/settings_test.go**: Update tests for renamed function `displayNoSettingsFoundMessage()` - **cmd/list/values_test.go**: Update tests for renamed function `displayNoValuesFoundMessage()` ### List metadata migration to renderer pipeline - **cmd/list/metadata.go**: Migrate to renderer pipeline (91 lines, down from 161) - **cmd/list/metadata_test.go**: Delete obsolete tests for old flag structure - **pkg/list/extract_metadata.go**: Create metadata extractor for renderer - **pkg/list/extract_metadata_test.go**: Add comprehensive tests (5 test cases) - **pkg/list/list_metadata.go**: Implement renderer integration with default columns ### Documentation updates - **website/docs/cli/commands/list/list-metadata.mdx**: Complete rewrite for table view with column configuration examples - **website/docs/cli/commands/list/list-instances.mdx**: New comprehensive documentation - **website/docs/cli/commands/list/list-vendor.mdx**: New comprehensive documentation - **website/docs/cli/commands/list/list-components.mdx**: Enhanced with column examples - **website/docs/cli/commands/list/list-stacks.mdx**: Add column configuration - **website/docs/cli/commands/list/list-workflows.mdx**: Fix duplicate sections - **website/blog/2025-11-17-customizable-list-command-output.mdx**: Feature announcement blog post ### Breaking changes for list metadata - Removed `--query` flag (use `--filter` with YQ syntax instead) - Removed `--max-columns` flag (use renderer table formatting) - Removed `--delimiter` flag (renderer handles formatting) - Removed positional component argument (use `--filter` for filtering) ## Resolution Fixes issue where `atmos list metadata` displayed "INFO No metadata found" instead of proper UI output. Now uses `ui.Info()` for user-facing messages and renderer pipeline for consistent table formatting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
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 |
Apply theme-aware color styling to table cells based on content type:
- Booleans: Green (Success) for 'true', Red (Error) for 'false'
- Numbers: Cyan (Info) for numeric values
- Placeholders: Darker gray (Muted) for {...} and [...] placeholders
- Default text: No color styling (uses base table cell style)
Implementation details:
- Add detectContentType() to identify cell content (boolean, number, placeholder)
- Add getCellStyle() to apply appropriate theme style based on content type
- Update CreateStyledTable() StyleFunc to apply semantic styling
- Use theme.GetCurrentStyles() for theme-aware color consistency
- Add regex pattern matching for placeholder detection ({...}, [...])
- Pass baseStyle by pointer to avoid copying heavy struct (552 bytes)
This enhances readability by visually distinguishing different data types
in list command table output (instances, metadata, workflows, etc.)
Tables now expand arrays of scalar values (strings, numbers, booleans)
into multi-row cells with one value per line, making array data readable
without needing to expand placeholders.
Changes:
- Add tryExpandScalarArray() to detect and expand scalar arrays
- Arrays with non-scalar elements still show placeholder format
- Add joinItems() helper to join array items with newlines
- Respect MaxColumnWidth for individual array items
- Maps continue to show {...} (N keys) placeholder format
Benefits:
- availability_zones: us-east-1a, us-east-1b, us-east-1c (multi-line)
- Complex arrays: [...] (N items) (placeholder for non-scalar)
- Improves readability for common scalar array fields
Tests:
- Add comprehensive tests for scalar array expansion
- Test mixed types, nested arrays, empty arrays
- Verify placeholder fallback for complex structures
Replace byte-based len() with lipgloss.Width() for proper handling of: - ANSI escape codes (colors, styles) - Multi-byte Unicode characters - Multi-line cell content (expanded arrays) Changes: - Replace len() with lipgloss.Width() in width calculations - Add getMaxLineWidth() to handle multi-line cells correctly - Add splitLines() helper for line-by-line width calculation - Update calculateMaxKeyWidth() to use visual width - Update calculateStackColumnWidth() to use visual width - Update getMaxValueWidth() to find widest line in multi-line values Benefits: - Tables now size columns based on actual visual width - Colored text no longer causes incorrect column sizing - Multi-line array expansions properly measured - More efficient use of terminal space Tests: - Add tests for getMaxLineWidth with ANSI codes - Add tests for splitLines with various inputs - Verify ANSI-aware width calculation
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
cmd/list/vendor.go (1)
145-171:--columnsfor vendor shares the same stub implementation
getVendorColumnsalso callsparseColumnsFlag(columnsFlag)when--columnsis set, but that helper is currently a TODO that always returns default columns. So for vendor:
--columns/ATMOS_LIST_COLUMNSdo not actually influence the output today.Once
parseColumnsFlagis implemented for components, it would be good to hook vendor into the same real implementation so behavior is consistent.
🧹 Nitpick comments (20)
CLAUDE.md (1)
39-74: Good addition - consider fixing the markdownlint issue.This section provides clear guidance on when to consult specialized agents. The examples and rationale are helpful.
One minor issue: The fenced code block at line 57 is missing a language identifier. Consider adding one for proper syntax highlighting:
-``` +```text Task( subagent_type: "flag-handler", prompt: "Help design named wrapper functions for list command flags" )</blockquote></details> <details> <summary>website/docs/cli/commands/list/list-vendor.mdx (1)</summary><blockquote> `24-37`: **Minor style nit in description paragraph** The “Target destination path” / “Vendor configuration file” bullets read a bit abruptly after “information about:”. If you care about polish, consider making the last bullet a full phrase like “Vendor configuration file path.” or similar, but the content is otherwise clear. </blockquote></details> <details> <summary>cmd/list/instances.go (1)</summary><blockquote> `15-26`: **InstancesOptions is built but not used downstream** `InstancesOptions` (including the new `Filter` and `Sort` fields) is populated in `RunE` and passed into `executeListInstancesCmd`, but `executeListInstancesCmd` ignores `opts` and just calls `list.ExecuteListInstancesCmd(&configAndStacksInfo, cmd, args)`. If the list package is still reading values directly from Cobra/Viper, this options struct isn’t buying you anything right now and may confuse future readers. I’d either: - Wire `opts` through into `list.ExecuteListInstancesCmd` (if you’re moving instances onto the same options-based renderer pipeline as other list commands), or - Drop `InstancesOptions` / the `opts` parameter for now and reintroduce it when instances actually consume these fields. Also applies to: 47-57, 85-95 </blockquote></details> <details> <summary>cmd/list/settings.go (1)</summary><blockquote> `116-123`: **Consider handling ui.Info errors.** The error returns from `ui.Info` are discarded. While this may be acceptable for informational messages, consider whether these errors should be logged or returned, especially if UI output failures could impact user experience. ```diff func displayNoSettingsFoundMessage(componentFilter string) { if componentFilter != "" { - _ = ui.Info("No settings found for component: " + componentFilter) + if err := ui.Info("No settings found for component: " + componentFilter); err != nil { + log.Debug("Failed to display no settings message", "error", err) + } } else { - _ = ui.Info("No settings found") + if err := ui.Info("No settings found"); err != nil { + log.Debug("Failed to display no settings message", "error", err) + } } }pkg/list/extract_metadata.go (1)
34-50: Simplify string concatenation with strings.Join.The manual string building for
inheritscan be streamlined usingstrings.Join, making the code more concise and idiomatic.if val, ok := instance.Metadata["inherits"].([]interface{}); ok { - // Convert []interface{} to comma-separated string. inheritsSlice := make([]string, 0, len(val)) for _, v := range val { if str, ok := v.(string); ok { inheritsSlice = append(inheritsSlice, str) } } - if len(inheritsSlice) > 0 { - for i, s := range inheritsSlice { - if i > 0 { - inherits += ", " - } - inherits += s - } - } + inherits = strings.Join(inheritsSlice, ", ") }Don't forget to import
stringsif not already imported.pkg/list/list_metadata.go (1)
48-85: Consider adding filter and sort support.The renderer is created with
nilfilters and sorters (line 77), but other list commands support filtering and sorting via CLI flags. Consider whether metadata listing should also support these features for consistency.If filtering and sorting are needed, you could add flags and build filters/sorters similar to other list commands:
func ExecuteListMetadataCmd(info *schema.ConfigAndStacksInfo, cmd *cobra.Command, args []string) error { // Initialize CLI config. atmosConfig, err := cfg.InitCliConfig(*info, true) if err != nil { return errors.Join(errUtils.ErrFailedToInitConfig, err) } - // Get flags. + // Get flags formatFlag, _ := cmd.Flags().GetString("format") + filterFlag, _ := cmd.Flags().GetString("filter") + sortFlag, _ := cmd.Flags().GetString("sort") // Process instances (same as list instances, but we'll extract metadata). instances, err := processInstances(&atmosConfig) if err != nil { return errors.Join(errUtils.ErrProcessInstances, err) } // Extract metadata into renderer-compatible format. data := ExtractMetadata(instances) // Get column configuration. columns := getMetadataColumns(&atmosConfig) + + // Build filters and sorters + filters := buildMetadataFilters(filterFlag) + sorters := buildMetadataSorters(sortFlag) // Create column selector. selector, err := column.NewSelector(columns, column.BuildColumnFuncMap()) if err != nil { return fmt.Errorf("failed to create column selector: %w", err) } // Create renderer. - r := renderer.New(nil, selector, nil, format.Format(formatFlag)) + r := renderer.New(filters, selector, sorters, format.Format(formatFlag)) // Render output. if err := r.Render(data); err != nil { return fmt.Errorf("failed to render metadata: %w", err) } return nil }cmd/list/flag_wrappers_test.go (1)
377-414: Tighten env var binding test or drop unusedenvVarName
TestFlagEnvironmentVariableBindingcurrently only asserts thatBindToVipersucceeds; theenvVarNamefield is never used, so the test doesn’t actually confirm the binding name.Either:
- Use
envVarName(e.g.,t.Setenv(tt.envVarName, "x"), callBindToViper, then assertv.GetString(tt.flagName) == "x"), or- Remove
envVarNamefrom the table to avoid implying stronger coverage than is present.pkg/list/extract_stacks.go (1)
9-73: Consider whether deterministic stack ordering is neededBoth extractors iterate over
stacksMapdirectly, so the output order is whatever Go’s map iteration yields. If the downstream renderer doesn’t always apply an explicit sort (e.g., default sort bystack), CLI output may appear in a non‑deterministic order between runs.If you want stable default ordering, consider sorting the stack names (or ensure callers always run the sort pipeline on these results). Otherwise, the current implementation is straightforward and matches the tests.
pkg/list/renderer/renderer_test.go (1)
38-237: Consider factoring I/O initialization into a helperMost tests duplicate the
NewContext/InitWriter/InitFormattersetup. Extracting a small helper likeinitTestRendererIO(t)would cut repetition and make future tweaks to the IO/UI wiring cheaper.pkg/list/extract_components.go (1)
9-17: Unused exportedComponentDatastruct
ComponentDataisn’t used in this file. If there’s no external consumer yet, consider either wiring it into the API (e.g., returning strongly‑typed slices instead of[]map[string]any) or dropping it for now to avoid a “blessed but unused” public type.pkg/list/extract_components_test.go (1)
11-245: Nice coverage; consider assertingcomponent_typesemantics explicitlyThe tests do a good job exercising success and failure paths. To lock in the intended meaning of metadata fields, you might add a couple of assertions that:
- For an abstract component (like
ingressinTestExtractComponents),comp["component_type"] == "abstract".- For a normal TF component,
comp["type"] == "terraform"andcomp["component_type"] == "real".That would make future refactors around
.typevs.component_typesafer.cmd/list/stacks.go (1)
29-31: Columns flag type vs parsing helper may not line up
StacksOptions.Columnsis astringpopulated viav.GetString("columns"), butWithColumnsFlagregisterscolumnsas aStringSliceflag. Depending on howStandardParserbinds to Viper,GetString("columns")may see something like"[stack,component]"rather than the rawstack,componentstring thatparseColumnsFlagexpects.If the intent is “comma‑separated single value”, you might either:
- Change the flag to a simple string flag; or
- Keep it as a slice and update
StacksOptions+parseColumnsFlagto work with[]string.Worth a quick manual check of
--columns stack,componentand the env var form to confirm the value that reachesparseColumnsFlag.Also applies to: 150-154
website/docs/cli/commands/list/list-stacks.mdx (1)
119-142: Clarify that the listed template functions are a subset of what’s availableThe “Available functions” list here is a subset of what
BuildColumnFuncMapexposes (pad,get,getOr,has,split,toInt,toBool, etc. are also supported). To avoid confusing folks who read the code, consider either:
- Renaming this to “Common functions” and noting it’s not exhaustive, or
- Expanding the list to match the actual func map.
pkg/list/renderer/renderer.go (1)
151-159: Use a CSV/TSV writer to handle escaping correctly
formatDelimitedcurrently does a rawstrings.Joinfor CSV/TSV. That means fields containing the delimiter, quotes, or newlines won’t be escaped and can break consumers.Consider switching to Go’s
encoding/csv(with the delimiter configurable for TSV) so quoting and escaping are handled correctly, e.g., by writing into abytes.Bufferand returning the buffer string.cmd/list/components.go (1)
142-177: Revisit interaction betweentypeandabstractfilters
buildComponentFilterscurrently combines:
Typeflag →filter.NewColumnFilter("type", opts.Type)(default"real"fromWithTypeFlag), andAbstractflag → when!opts.Abstract, always addsfilter.NewColumnFilter("component_type", "real").This can make it hard to express some combinations:
- With no flags you filter on both
type=realandcomponent_type=real.- With
--abstractbut no explicit--type, you still filtertype=real, so abstract-only components may never show.- With
--type=allbut--abstractomitted, you still filtercomponent_typetoreal, so “all” doesn’t actually include abstract.Might be worth clarifying which field (
typevscomponent_type) is the canonical “real/abstract/all” dimension and simplifying defaults around that (e.g., letTypedefault to""and derive the “real-only by default” behavior solely fromAbstract).cmd/list/vendor.go (2)
130-143: Confirm intent of using--stackto glob on thecomponentfield
buildVendorFilterstreats the--stackflag as a glob on the"component"field:globFilter, err := filter.NewGlobFilter("component", opts.Stack)Given
WithStackFlag’s help text (“Filter by stack pattern”) this is a bit surprising. If vendor rows don’t have astackfield and the goal is to filter by component name, it might be clearer to either:
- Introduce a dedicated
--componentfilter for vendor, or- Rename/update the
--stackhelp text for this command.At minimum, worth double-checking that this aligns with how users expect to filter vendor entries.
185-256: Unused home-dir obfuscation helpers after renderer refactorAfter switching vendor listing to the renderer pipeline,
obfuscateHomeDirInOutput,shouldReplaceHomeDir, andisPathCharare no longer used in this file.If the renderer-based output is intentionally allowed to show absolute paths now, these helpers can probably be removed. If you still want to avoid leaking
$HOMEin vendor paths, you may want to integrate this behavior into the data extraction step or the renderer instead.pkg/list/sort/sort.go (1)
110-133: Numeric and multi-column sort behavior is good; tighten comments around edge casesThe type-aware sorting and stable multi-column precedence look solid. Two small nits:
compareNumeric’s comment says “Non-numeric values sort last”, but when both values are non-numeric you fall back tostrings.Compare(a, b). That’s fine behavior, just worth reflecting in the comment so future readers don’t assume “always last”.- In
Sorter.Sort, rows wherecolIdx >= len(row)are effectively treated as “equal” to everything else (you always returnfalsefrom the comparator). That’s a reasonable choice; consider documenting that missing-column rows preserve original order and don’t participate in the sort.No functional changes required; this is just about making the behavior explicit.
Also applies to: 161-185
pkg/list/filter/filter.go (1)
39-86: Filter primitives and chaining look solidThe glob/column/boolean filters and the Chain combinator are straightforward and type-safe, with good validation and error messages. The permissive string handling in
BoolFilter(treating non-true strings as false) is fine for the intendedenabled/lockeduse cases; if you later apply it to arbitrary fields you might want stricter validation, but nothing blocking here.Nice foundation for composing filters into the renderer pipeline.
Also applies to: 119-160, 167-178
pkg/list/column/column.go (1)
27-47: TemplateContext struct is unused; current implementation relies on a generic map-based contextRight now, templates are evaluated against the
map[string]anyproduced bybuildTemplateContext, and theTemplateContextstruct isn’t used anywhere. That’s not wrong, but it’s a bit misleading for readers who might assume templates see a strongly-typed context with the JSON-tagged field names.You can either:
- Remove
TemplateContextuntil you actually switch to using it, or- Update
buildTemplateContextto construct and return aTemplateContext(plus maybe embedRawseparately) so the declared API matches the real context shape.Low priority, but it’ll reduce cognitive overhead for anyone extending the column templating later.
Also applies to: 190-272
Tables now expand maps with scalar values (like tags) into readable
multi-line format with one key-value pair per line.
Before:
tags {...} (7 keys)
After:
tags Environment: production
Name: vpc
Team: platform
...
Implementation:
- Add tryExpandScalarMap() to detect and expand scalar maps
- Sort keys alphabetically for consistent ordering
- Format as 'key: value' pairs, one per line
- Maps with non-scalar values still show {...} (N keys) placeholder
- Shares joinItems() helper with array expansion
Behavior:
- Scalar maps (string, number, bool values) → Expanded multi-line
- Complex maps (nested objects, arrays) → Placeholder {...} (N keys)
- Empty maps → Empty string
- Keys sorted alphabetically for consistency
Benefits:
- Tags and metadata now readable without expanding
- Consistent with array expansion behavior
- Maintains compact placeholder for complex structures
Tests:
- Add comprehensive tests for scalar map expansion
- Test mixed scalar types, empty maps
- Verify placeholder fallback for nested structures
- Update existing tests to match new behavior
Prevent scalar arrays and maps from being expanded if their items
exceed a reasonable width threshold. This avoids creating impossibly
wide tables that trigger "table too wide" errors.
Problem:
- Expanding arrays/maps with long values caused tables to exceed terminal width
- Users saw: "Error: the table is too wide to display properly (width: 418 > 185)"
- No fallback to compact placeholder format for wide values
Solution:
- Track maximum item width during expansion
- Reject expansion if any item exceeds maxReasonableItemWidth (20 chars)
- Fall back to compact placeholder format: [...] (N items) or {...} (N keys)
- Threshold set to ~1/3 of MaxColumnWidth for reasonable table display
Changes:
- Add width tracking to tryExpandScalarArray()
- Add width tracking to tryExpandScalarMap()
- Use lipgloss.Width() for ANSI-aware measurement
- Set maxReasonableItemWidth constant to 20 characters
Benefits:
- Tables stay within terminal width limits
- Short values still expand (availability_zones, tags with short values)
- Long values gracefully degrade to placeholders
- No more "table too wide" errors from expansion
Tests:
- Add test for arrays with very long strings
- Add test for maps with very long values
- Verify both fall back to placeholder format
- Add renderInlineMarkdown() function to render markdown in table cells - Uses glamour library with theme-aware styling for terminals - Automatically detects color support and degrades gracefully - Collapses multi-line markdown into single line for table cells - Applies to Description column specifically when present in headers - Preserves inline markdown (bold, italic, code) while removing blocks - Includes comprehensive tests for markdown rendering edge cases Resolves markdown rendering for description fields in metadata tables.
…data
**Enhancement 1: Style <no value> in Dark Gray**
- Add contentTypeNoValue enum to cell content type detection
- Update detectContentType() to identify <no value> from Go templates
- Apply styles.Muted (dark gray) styling to <no value> cells
- Consistent with placeholder styling ({...}, [...])
- Theme-aware and zero-config
**Enhancement 2: Support Vars in List Metadata**
- Expose vars, settings, and env in ExtractMetadata()
- Enables column templates to access component variables
- Users can now use {{ .vars.region }}, {{ .vars.environment }}, etc.
- Follows same pattern as ExtractInstances()
**Testing:**
- Add TestDetectContentType_NoValue with comprehensive test cases
- Add TestExtractMetadata_IncludesVarsSettingsEnv to verify exposure
- Update existing metadata tests to include new fields
- All tests passing
**Impact:**
- list metadata now displays actual var values instead of <no value>
- Improves readability with dark gray styling for missing values
- Enables custom column configuration using vars/settings/env
Closes user request for dark gray <no value> styling and vars support.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/prd/auth-context-multi-identity.md (1)
559-569: Fix markdownlint hard‑tab warning.markdownlint is flagging a hard tab around this section (MD010,
no-hard-tabs). Swapping that leading tab for spaces will clear the lint error without affecting rendering.
♻️ Duplicate comments (2)
cmd/list/components.go (1)
285-299:--columnsflag still doesn’t influence output; only a warning is emitted.
parseColumnsFlagcurrently just logsCustom --columns parsing not yet implemented, using defaultsand returns the defaultComponent/Stack/Typeconfig, so any--columnsorATMOS_LIST_COLUMNSusage won’t actually change the rendered columns.Given the PR advertises configurable columns via
--columns, you probably want either:
- A minimal implementation that actually maps
columnsFlagto a[]column.Config(even if initially limited to a small set of known columns), or- A stricter behavior that errors when
--columnsis set until the feature is ready, to avoid surprising users.As-is, the warning helps, but the flag is effectively non-functional.
cmd/list/stacks.go (1)
231-297: Stacks--columnsflag shares the same “stubbed” behavior as components.
getStackColumnsdefers toparseColumnsFlagwhencolumnsFlagis non-empty, butparseColumnsFlagonly warns and returns the default column set. That meansatmos list stacks --columns ...doesn’t actually alter the rendered columns yet, just likelist components.This is fine as an interim step, but it’s worth aligning expectations:
- Either wire stacks into the real column parsing once you implement it for components, or
- Clearly document that
--columnsis currently config‑only (viastacks.list.columnsin atmos.yaml) and that the flag is experimental.No blocking change required if you plan to follow up soon, but it’s good to be explicit since this flag now appears in the CLI.
🧹 Nitpick comments (2)
internal/exec/stack_processor_utils.go (1)
584-597: Consider adding a godoc comment.While the struct is well-named and its fields are clear from context, a brief godoc comment would improve maintainability:
// importFileResult holds the result of processing a single import file in parallel. // Results are collected from parallel processing and merged sequentially to preserve // Atmos inheritance order. type importFileResult struct {cmd/list/stacks.go (1)
150-153: Tiny consistency nit: reuse the format constant for the tree check.Here you compare
opts.Formatto the literal"tree", whilevalidateProvenanceFlagandinitStacksConfigusestring(format.FormatTree). Swapping this check to use the constant as well would keep all format comparisons centralized and avoid drift if the enum ever changes.Something like:
- if opts.Format == "tree" { + if opts.Format == string(format.FormatTree) { return renderStacksTreeFormat(&atmosConfig, stacks, opts.Provenance, authManager) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
NOTICE(2 hunks)cmd/list/components.go(1 hunks)cmd/list/stacks.go(1 hunks)docs/prd/auth-context-multi-identity.md(1 hunks)errors/errors.go(3 hunks)go.mod(4 hunks)internal/aws_utils/aws_utils.go(1 hunks)internal/exec/stack_processor_utils.go(34 hunks)pkg/auth/cloud/aws/env.go(2 hunks)pkg/auth/providers/aws/sso.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- NOTICE
- go.mod
🧰 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:
internal/aws_utils/aws_utils.gopkg/auth/providers/aws/sso.gopkg/auth/cloud/aws/env.goerrors/errors.gocmd/list/stacks.gocmd/list/components.gointernal/exec/stack_processor_utils.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/auth/providers/aws/sso.gopkg/auth/cloud/aws/env.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/list/stacks.gocmd/list/components.go
docs/prd/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for PRD filenames:
command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md. All PRDs indocs/prd/
Files:
docs/prd/auth-context-multi-identity.md
🧠 Learnings (68)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: 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: osterman
Repo: cloudposse/atmos PR: 1533
File: pkg/config/load.go:585-637
Timestamp: 2025-09-27T20:50:20.564Z
Learning: In the cloudposse/atmos repository, command merging prioritizes precedence over display ordering. Help commands are displayed lexicographically regardless of internal array order, so the mergeCommandArrays function focuses on ensuring the correct precedence chain (top-level file wins) rather than maintaining specific display order.
📚 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/aws_utils/aws_utils.godocs/prd/auth-context-multi-identity.md
📚 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/aws_utils/aws_utils.gointernal/exec/stack_processor_utils.godocs/prd/auth-context-multi-identity.md
📚 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/auth/cloud/aws/env.gocmd/list/stacks.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 : 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-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use `assert.ErrorIs(t, err, ErrSentinel)` for checking our own errors and stdlib errors. Only use string matching for third-party errors
Applied to files:
errors/errors.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:
cmd/list/stacks.gocmd/list/components.gointernal/exec/stack_processor_utils.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:
cmd/list/stacks.gointernal/exec/stack_processor_utils.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/list/stacks.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/list/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 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/list/stacks.gocmd/list/components.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/list/stacks.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:
cmd/list/stacks.gointernal/exec/stack_processor_utils.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/list/stacks.gocmd/list/components.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/list/stacks.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:
cmd/list/stacks.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:
cmd/list/stacks.gocmd/list/components.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.gocmd/list/components.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/list/stacks.gocmd/list/components.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
cmd/list/stacks.gointernal/exec/stack_processor_utils.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:
cmd/list/stacks.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 : Never use `fmt.Fprintf(os.Stdout/Stderr, ...)`, `fmt.Println()`, or direct stream access for I/O. Always use `data.*` for pipeable output or `ui.*` for user-facing messages
Applied to files:
cmd/list/stacks.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 : For output: Pipeable data → `data.*` (stdout), Plain UI → `ui.Write*()` (stderr), Status messages → `ui.Success/Error/Warning/Info()` (stderr), Formatted docs → `ui.Markdown*()`. Use `data.Write()`, `data.Writef()`, `data.Writeln()`, `data.WriteJSON()`, `data.WriteYAML()` for data output
Applied to files:
cmd/list/stacks.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/io/**/*.go : Use I/O layer (`pkg/io/`) for stream access (stdout/stderr/stdin), terminal capabilities, masking. Use UI layer (`pkg/ui/`) for formatting (colors, styles, markdown rendering). Never use `fmt.Fprintf(os.Stdout/Stderr, ...)`, `fmt.Println()`, or direct stream access
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.gointernal/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:
cmd/list/stacks.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/list/stacks.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 tests/**/*_test.go : NEVER use pipe redirection (`2>&1`, `| head`, `| tail`) when running tests. Piping breaks TTY detection → ASCII fallback → wrong snapshots. NEVER manually edit golden snapshot files - always use `-regenerate-snapshots` flag. Snapshots capture exact output including invisible formatting (lipgloss padding, ANSI codes, trailing whitespace)
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.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 : ALWAYS ask for user approval before adding nolint comments. Prefer refactoring over nolint - only use nolint as last resort with explicit user permission. Exception for bubbletea models: `//nolint:gocritic // bubbletea models must be passed by value`. Exception for intentional subprocess calls: `//nolint:gosec // intentional subprocess call`. NEVER add nolint for gocognit, cyclomatic complexity, magic numbers, or nestif - refactor instead
Applied to files:
cmd/list/stacks.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 : Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`
Applied to files:
cmd/list/stacks.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
cmd/list/stacks.gointernal/exec/stack_processor_utils.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:
cmd/list/stacks.gocmd/list/components.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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 : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions. Use `nil` if no atmosConfig param
Applied to files:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.gointernal/exec/stack_processor_utils.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:
cmd/list/stacks.gocmd/list/components.go
📚 Learning: 2024-12-08T14:26:16.972Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 828
File: pkg/schema/schema.go:98-100
Timestamp: 2024-12-08T14:26:16.972Z
Learning: The `ListConfig` columns array in the `Components` struct can be empty.
Applied to files:
cmd/list/components.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 functional options pattern for configuration instead of many parameters. Provide defaults and extensibility without breaking changes. Pattern: `type Option func(*Config)`, `func WithName(val) Option { return func(c *Config) { c.Name = val } }`, `func New(opts ...Option) { cfg := &Config{/*defaults*/}; for _, opt := range opts { opt(cfg) }; return &Client{config: cfg} }`
Applied to files:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-06-07T19:28:21.289Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: cmd/describe_affected.go:0-0
Timestamp: 2025-06-07T19:28:21.289Z
Learning: In the Atmos codebase, using panic for unsupported flag types in flag processing functions like setDescribeAffectedFlagValueInCliArgs is the expected behavior rather than returning errors. This pattern is preferred for developer errors when unsupported types are added to the flagsKeyValue map.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/list/components.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 : Use `flags.NewStandardParser()` for command-specific flag parsing. NEVER call `viper.BindEnv()` or `viper.BindPFlag()` directly - use the unified `pkg/flags/` infrastructure instead. Register flags with Cobra command and bind to Viper for precedence handling
Applied to files:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-02-09T14:38:53.443Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/cmd_utils.go:0-0
Timestamp: 2025-02-09T14:38:53.443Z
Learning: Error handling for RegisterFlagCompletionFunc in AddStackCompletion is not required as the errors are non-critical for tab completion functionality.
Applied to files:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer. This is necessary for proper handling of recursive directory matching when using the gobwas/glob library.
Applied to files:
cmd/list/components.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_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 : 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-10-16T05:26:31.892Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1639
File: pkg/merge/merge.go:392-430
Timestamp: 2025-10-16T05:26:31.892Z
Learning: In Atmos merge operations (pkg/merge/merge.go), context.Positions is a PositionMap (map[string]*schema.Position) keyed by JSONPath strings, not a slice indexed by input array position. Position lookups are path-based, making them independent of input array reorganization or filtering. This design allows filtering empty inputs without affecting provenance tracking correctness.
Applied to files:
internal/exec/stack_processor_utils.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:
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 : Before implementing new functionality, search `internal/exec/` and `pkg/` for existing code. Extend, don't duplicate
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 **/*_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:
internal/exec/stack_processor_utils.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_utils.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
internal/exec/stack_processor_utils.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:
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-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-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:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-03-17T18:41:08.831Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1085
File: pkg/config/imports.go:68-75
Timestamp: 2025-03-17T18:41:08.831Z
Learning: In the Atmos configuration import system, errors during config file merging are logged at debug level and the process continues with other imports rather than failing completely, prioritizing resilience over strict correctness.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-04-03T10:57:04.602Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1091
File: pkg/config/load_config_args.go:61-82
Timestamp: 2025-04-03T10:57:04.602Z
Learning: In the Atmos configuration system, errors in `mergeDefaultImports` and `mergeImports` functions are intentionally only logged as debug messages without returning errors, as they are considered non-critical. Only errors in critical functions like `mergeConfigFile` are returned to halt execution.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
docs/prd/auth-context-multi-identity.md
🧬 Code graph analysis (3)
pkg/auth/providers/aws/sso.go (3)
errors/builder.go (1)
Build(24-37)errors/errors.go (1)
ErrLoadAWSConfig(110-110)pkg/store/errors.go (1)
ErrLoadAWSConfig(23-23)
pkg/auth/cloud/aws/env.go (2)
errors/errors.go (1)
ErrLoadAWSConfig(110-110)pkg/store/errors.go (1)
ErrLoadAWSConfig(23-23)
cmd/list/stacks.go (7)
pkg/list/format/formatter.go (2)
Format(8-8)FormatTree(17-17)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/list/extract/stacks.go (2)
Stacks(11-27)StacksForComponent(30-73)cmd/list/flag_wrappers.go (6)
NewListParser(304-315)WithFormatFlag(32-40)WithStacksColumnsFlag(91-98)WithSortFlag(149-156)WithComponentFlag(197-204)WithProvenanceFlag(285-292)pkg/list/column/column.go (2)
NewSelector(52-79)Config(13-17)pkg/list/renderer/renderer.go (1)
New(31-46)pkg/list/sort/sort.go (4)
Sorter(35-39)NewSorter(48-54)Ascending(17-17)ParseSortSpec(195-237)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/auth-context-multi-identity.md
559-559: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (8)
internal/exec/stack_processor_utils.go (4)
83-97: Clean implementation with proper thread safety.The function correctly uses read locks, returns a defensive copy to prevent external mutations, and includes performance tracking. Good work.
878-879: Request approval for nolint directive.Per coding guidelines: "ALWAYS ask for user approval before adding nolint comments. Prefer refactoring over nolint - only use nolint as last resort with explicit user permission."
The nil check for
atmosConfigis present in the guard condition on line 879. If staticcheck is flagging this as an issue, consider alternative approaches:
- Extract the trace call into a helper that handles nil atmosConfig
- Restructure the condition to make the nil check more explicit to the linter
Do you approve adding this nolint directive, or would you prefer a refactor?
1057-1058: Well-refactored provenance tracking.The extraction of provenance logic into
processImportProvenanceTrackingsignificantly improves readability and addresses the previous review feedback about removing the nestif nolint directive.
1875-1924: Excellent refactoring of provenance tracking logic.Both helper functions are well-designed:
- Clear, focused responsibilities
- Proper nil checks with early returns
- Appropriate trace logging for debugging
- Performance tracking in place
- Good documentation
This successfully addresses the previous review comment about extracting the nested provenance logic and removing the nolint:nestif directive.
errors/errors.go (1)
48-49: New error sentinels look consistent and well-scoped.
ErrTemplateEvaluation,ErrInvalidConfig,ErrLoadAWSConfig, andErrNoStacksFoundfollow the existing naming/message pattern and line up with how templates, config validation, AWS config loading, and list extract paths use them. No changes needed here.Also applies to: 110-110, 453-453
internal/aws_utils/aws_utils.go (1)
94-100: AWS config load error now uses the unified sentinel.Wrapping the
config.LoadDefaultConfigfailure withErrLoadAWSConfigkeeps this path consistent with the new sentinel across the repo while preserving the underlying error. Looks good.pkg/auth/cloud/aws/env.go (1)
115-121: Consistent use ofErrLoadAWSConfigacross AWS env loaders.Both
LoadIsolatedAWSConfigandLoadAtmosManagedAWSConfignow wrap failures withErrLoadAWSConfig, matching the central sentinel and other AWS helpers. This keeps error handling uniform without altering behavior.Also applies to: 181-184
pkg/auth/providers/aws/sso.go (1)
142-156: SSO auth now reports AWS config load failures viaErrLoadAWSConfig.Using
errUtils.Build(errUtils.ErrLoadAWSConfig)here aligns SSO’s error path with the updated sentinel and the rest of the AWS helpers, while still adding good explanation/hint/context via the builder. No further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cmd/list/components.go (1)
311-325:parseColumnsFlagremains a stub but now warns users.The TODO is acceptable since users are informed via
ui.Warningthat custom columns aren't parsed yet. This provides transparency rather than silent failure. Consider tracking this as a follow-up issue.Want me to open an issue to track the
--columnsparsing implementation?cmd/list/components_test.go (1)
650-654: Consider using sentinel errors for error checking.Line 653 uses
assert.Contains(err.Error(), tc.errorMsg)which the coding guidelines discourage. IflistSort.ParseSortSpecreturns a sentinel error, preferassert.ErrorIs(). If not, this string match is acceptable for now.internal/exec/stack_processor_utils.go (1)
617-618: Clarify or remove the staticcheck nolint directive.The comment says "atmosConfig nil check is present" but line 618 uses
atmosConfig != nil && atmosConfig.TrackProvenancewhich is valid Go. Thestaticchecklinter shouldn't flag this. If this suppresses a false positive, consider verifying it's still needed.internal/exec/stack_processor_cache.go (1)
193-210: Type mismatch between stored and returned content.Line 207 stores
contentas[]byte, but line 200 usesfmt.Sprintf("%s", existingContent)to convert. This works but is inefficient. Consider storing asstringdirectly or using type assertion.- content, err := os.ReadFile(filePath) - if err != nil { - return "", err - } - getFileContentSyncMap.Store(filePath, content) - - return string(content), nil + content, err := os.ReadFile(filePath) + if err != nil { + return "", err + } + contentStr := string(content) + getFileContentSyncMap.Store(filePath, contentStr) + + return contentStr, nilAnd update the retrieval:
existingContent, found := getFileContentSyncMap.Load(filePath) if found && existingContent != nil { - return fmt.Sprintf("%s", existingContent), nil + return existingContent.(string), nil }internal/exec/stack_processor_cache_test.go (1)
305-324: Consider testing with a real compiled schema.The test caches and retrieves
nil, which verifies the mechanism but doesn't confirm real schema objects work correctly. If schema compilation is testable, a follow-up test with an actual schema would strengthen coverage.internal/exec/stack_processor_provenance_test.go (1)
86-93: Test validates map copy but not deep copy of values.The test confirms that modifying the returned map doesn't affect the original store, but note that
GetAllMergeContextsreturns the same*m.MergeContextpointers. If callers mutate those, it would affect the stored contexts. This might be intentional for performance, but worth documenting.internal/exec/stack_processor_provenance.go (1)
51-65: Shallow copy may allow external mutation of stored contexts.The comment says "prevent external modifications," but returning the same
*m.MergeContextpointers means callers can still mutate the context objects. If true immutability is needed, consider returning deep copies or documenting this behavior.If deep copy is needed:
// Return a copy to prevent external modifications. result := make(map[string]*m.MergeContext, len(mergeContexts)) for k, v := range mergeContexts { - result[k] = v + // Deep copy the context if immutability is required + result[k] = v // TODO: consider v.DeepCopy() if available } return resultOtherwise, update the comment to clarify the shallow copy semantics.
📜 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 (8)
.github/workflows/dependency-review.yml(1 hunks)cmd/list/components.go(1 hunks)cmd/list/components_test.go(2 hunks)internal/exec/stack_processor_cache.go(1 hunks)internal/exec/stack_processor_cache_test.go(1 hunks)internal/exec/stack_processor_provenance.go(1 hunks)internal/exec/stack_processor_provenance_test.go(1 hunks)internal/exec/stack_processor_utils.go(32 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/dependency-review.yml
🧰 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:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_provenance.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.gocmd/list/components.gocmd/list/components_test.gointernal/exec/stack_processor_utils.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/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_test.gocmd/list/components_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/list/components.gocmd/list/components_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
cmd.NewTestKit(t)for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Files:
cmd/list/components_test.go
🧠 Learnings (65)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: 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-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_test.gocmd/list/components_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_test.gocmd/list/components_test.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_provenance_test.gointernal/exec/stack_processor_provenance.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.gointernal/exec/stack_processor_utils.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_provenance.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_test.gocmd/list/components_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage. Tests must call actual production code, never duplicate logic. Target >80% test coverage (enforced by CodeCov)
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_test.gocmd/list/components_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:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.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 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:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache_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/stack_processor_provenance_test.gointernal/exec/stack_processor_provenance.gointernal/exec/stack_processor_cache.gocmd/list/components.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 : Before implementing new functionality, search `internal/exec/` and `pkg/` for existing code. Extend, don't duplicate
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.gointernal/exec/stack_processor_utils.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/stack_processor_provenance_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/stack_processor_provenance_test.go
📚 Learning: 2025-10-16T05:26:31.892Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1639
File: pkg/merge/merge.go:392-430
Timestamp: 2025-10-16T05:26:31.892Z
Learning: In Atmos merge operations (pkg/merge/merge.go), context.Positions is a PositionMap (map[string]*schema.Position) keyed by JSONPath strings, not a slice indexed by input array position. Position lookups are path-based, making them independent of input array reorganization or filtering. This design allows filtering empty inputs without affecting provenance tracking correctness.
Applied to files:
internal/exec/stack_processor_provenance_test.gointernal/exec/stack_processor_provenance.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 : 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_provenance.gocmd/list/components.gointernal/exec/stack_processor_utils.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:
internal/exec/stack_processor_provenance.gointernal/exec/stack_processor_utils.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_cache.gointernal/exec/stack_processor_cache_test.gointernal/exec/stack_processor_utils.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_cache.gointernal/exec/stack_processor_cache_test.gointernal/exec/stack_processor_utils.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:
internal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.gointernal/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_cache.gointernal/exec/stack_processor_cache_test.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/stack_processor_cache.gocmd/list/components.gocmd/list/components_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/stack_processor_cache.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:
internal/exec/stack_processor_cache.gointernal/exec/stack_processor_utils.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: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
internal/exec/stack_processor_cache.gointernal/exec/stack_processor_cache_test.go
📚 Learning: 2024-12-08T14:26:16.972Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 828
File: pkg/schema/schema.go:98-100
Timestamp: 2024-12-08T14:26:16.972Z
Learning: The `ListConfig` columns array in the `Components` struct can be empty.
Applied to files:
cmd/list/components.gocmd/list/components_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 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/list/components.gocmd/list/components_test.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/list/components.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/list/components.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:
cmd/list/components.go
📚 Learning: 2025-06-07T19:28:21.289Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: cmd/describe_affected.go:0-0
Timestamp: 2025-06-07T19:28:21.289Z
Learning: In the Atmos codebase, using panic for unsupported flag types in flag processing functions like setDescribeAffectedFlagValueInCliArgs is the expected behavior rather than returning errors. This pattern is preferred for developer errors when unsupported types are added to the flagsKeyValue map.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
cmd/list/components.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:
cmd/list/components.gocmd/list/components_test.gointernal/exec/stack_processor_utils.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/list/components.gocmd/list/components_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 cmd/**/*.go : Use `flags.NewStandardParser()` for command-specific flag parsing. NEVER call `viper.BindEnv()` or `viper.BindPFlag()` directly - use the unified `pkg/flags/` infrastructure instead. Register flags with Cobra command and bind to Viper for precedence handling
Applied to files:
cmd/list/components.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:
cmd/list/components.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:
cmd/list/components.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:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-02-09T14:38:53.443Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/cmd_utils.go:0-0
Timestamp: 2025-02-09T14:38:53.443Z
Learning: Error handling for RegisterFlagCompletionFunc in AddStackCompletion is not required as the errors are non-critical for tab completion functionality.
Applied to files:
cmd/list/components.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:
cmd/list/components.gointernal/exec/stack_processor_utils.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:
cmd/list/components.gointernal/exec/stack_processor_utils.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:
cmd/list/components.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer. This is necessary for proper handling of recursive directory matching when using the gobwas/glob library.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
cmd/list/components.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/list/components.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/list/components.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/list/components.gointernal/exec/stack_processor_utils.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:
cmd/list/components.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/**/*_test.go : ALWAYS use `cmd.NewTestKit(t)` for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Applied to files:
cmd/list/components_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:
cmd/list/components_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:
cmd/list/components_test.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
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 **/*_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:
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 : Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`
Applied to files:
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 : ALWAYS ask for user approval before adding nolint comments. Prefer refactoring over nolint - only use nolint as last resort with explicit user permission. Exception for bubbletea models: `//nolint:gocritic // bubbletea models must be passed by value`. Exception for intentional subprocess calls: `//nolint:gosec // intentional subprocess call`. NEVER add nolint for gocognit, cyclomatic complexity, magic numbers, or nestif - refactor instead
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 : Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
internal/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:
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: 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/stack_processor_utils.go
📚 Learning: 2025-03-17T18:41:08.831Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1085
File: pkg/config/imports.go:68-75
Timestamp: 2025-03-17T18:41:08.831Z
Learning: In the Atmos configuration import system, errors during config file merging are logged at debug level and the process continues with other imports rather than failing completely, prioritizing resilience over strict correctness.
Applied to files:
internal/exec/stack_processor_utils.go
📚 Learning: 2025-04-03T10:57:04.602Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1091
File: pkg/config/load_config_args.go:61-82
Timestamp: 2025-04-03T10:57:04.602Z
Learning: In the Atmos configuration system, errors in `mergeDefaultImports` and `mergeImports` functions are intentionally only logged as debug messages without returning errors, as they are considered non-critical. Only errors in critical functions like `mergeConfigFile` are returned to halt execution.
Applied to files:
internal/exec/stack_processor_utils.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/stack_processor_utils.go
🧬 Code graph analysis (4)
internal/exec/stack_processor_provenance_test.go (4)
internal/exec/stack_processor_provenance.go (6)
SetMergeContextForStack(25-31)GetMergeContextForStack(34-40)GetAllMergeContexts(53-65)ClearLastMergeContext(89-95)GetLastMergeContext(79-85)SetLastMergeContext(69-75)pkg/merge/merge_context.go (1)
NewMergeContext(39-43)pkg/schema/schema.go (1)
AtmosConfiguration(53-96)cmd/cmd_utils.go (1)
Contains(1212-1219)
internal/exec/stack_processor_provenance.go (5)
pkg/merge/merge_context.go (1)
MergeContext(16-36)pkg/perf/perf.go (1)
Track(121-138)pkg/schema/schema.go (1)
AtmosConfiguration(53-96)pkg/logger/log.go (1)
Trace(14-16)pkg/utils/slice_utils.go (1)
SliceContainsString(12-21)
internal/exec/stack_processor_cache.go (4)
pkg/schema/schema.go (1)
BaseComponentConfig(845-860)pkg/perf/perf.go (1)
Track(121-138)pkg/merge/merge.go (1)
DeepCopyMap(29-45)pkg/store/store.go (1)
Store(6-10)
cmd/list/components_test.go (3)
pkg/ui/formatter.go (1)
InitFormatter(47-69)pkg/data/data.go (1)
InitWriter(29-35)pkg/schema/schema.go (2)
ListConfig(971-976)ListColumnConfig(978-982)
⏰ 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). (4)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (20)
cmd/list/components.go (4)
1-25: Imports and package structure look clean.Good import organization with proper grouping. The errUtils, perf, and list subsystem packages are correctly imported for the new pipeline approach.
58-81: Tri-state flag handling is correctly implemented.Using
v.IsSet()beforev.GetBool()properly distinguishes unset (nil) from explicit true/false. This addresses the prior concern about default false being mistaken for user-set false.
161-199: Config initialization now uses ProcessCommandLineArgs.This aligns with other list subcommands (stacks, instances, metadata) for consistent CLI flag handling. Good error wrapping with sentinel errors.
230-265: Filter logic is sound.Type filter correctly targets
component_type(real/abstract distinction) rather than the backend type field. Glob pattern errors now warn users instead of silently ignoring. Tri-state boolean filters use proper nil checks.cmd/list/components_test.go (5)
15-25: Good test I/O initialization pattern.The
initTestIOhelper properly sets up I/O and UI contexts for render testing. Usingt.Helper()is correct for test helper functions.
138-230: Comprehensive options struct testing.Good coverage of tri-state boolean combinations (nil, true, false) and all field permutations. Table-driven approach is clean.
232-325: Filter building tests are solid.Tests correctly verify filter count expectations for each option combination. Covers edge cases like
Type="all"andAbstract=truewhich suppress the default real filter.
662-806: Good filter integration testing.These tests exercise the full render pipeline with various filter combinations, validating that tri-state booleans and type filters work through to the renderer without errors.
808-811: Good documentation of test boundaries.This comment correctly explains why unit testing
initAndExtractComponentsisn't meaningful and points to integration tests for full command flow coverage.internal/exec/stack_processor_utils.go (2)
323-336: Well-structured result type for parallel import processing.The
importFileResultstruct cleanly encapsulates all data needed from parallel import file processing. Good inclusion ofmergeContextfor provenance tracking.
796-798: Good extraction of provenance tracking logic.This addresses the previous review comment about nested if statements. The helper function
processImportProvenanceTrackingkeeps the main processing loop clean.internal/exec/stack_processor_cache.go (1)
15-30: Solid thread-safe cache setup.Good use of
sync.RWMutexfor the component and schema caches where read-heavy access is expected, andsync.Mapfor file content. The comments about no cache invalidation being needed per command execution are helpful.internal/exec/stack_processor_cache_test.go (2)
141-205: Thorough validation of cache behavior and field preservation.Good coverage of all
BaseComponentConfigfields including nested maps and the inheritance chain. The assertions verify the cache correctly stores and retrieves complex data.
207-245: Excellent deep copy verification.This test correctly validates that mutations to the original config after caching don't affect cached values, and that retrieved copies are independent. This is exactly what's needed for cache integrity.
internal/exec/stack_processor_provenance_test.go (3)
130-183: Solid concurrency stress test.Good coverage of concurrent Set, Get, and GetAll operations. The test doesn't assert specific outcomes (which is fine for race detection) - running with
-raceflag will catch any data races.
260-288: Good integration test for parent import chain updates.This test verifies the end-to-end behavior of
processImportProvenanceTrackingincluding how child imports propagate to the parent context.
313-331: Clean duplicate handling test.The counting approach at lines 323-329 is a clear way to verify duplicates aren't added. Good.
internal/exec/stack_processor_provenance.go (3)
127-146: Solid import chain propagation logic.Clean implementation using
SliceContainsStringfor duplicate prevention. The logging distinguishes direct vs. nested imports which aids debugging.For very large import chains, the O(n) lookups could add up. If performance becomes a concern, a map-based set could optimize this, but current implementation is clear and correct.
97-125: Well-structured provenance tracking helper.Good defensive programming with early returns for nil checks and disabled provenance. The logging at trace level provides visibility without noise.
67-95: Clear deprecation notices on legacy functions.Good practice to mark these as deprecated while maintaining backward compatibility. Consider adding a TODO with a timeline for removal if applicable.
Addresses CodeRabbit review comments #53 and #54. The BaseComponentMetadata field was missing from deep copy operations in getCachedBaseComponentConfig and cacheBaseComponentConfig, which could cause cache corruption if the metadata map was mutated after caching/retrieval. Refactored to extract deep copy logic into a helper function to reduce cyclomatic complexity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…osterman/list-ui-overhaul # Conflicts: # internal/exec/stack_processor_cache.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/list/components.go (1)
28-38: Useflag.Changedinstead ofv.IsSet()to detect user-set tri-state flagsThe current tri-state logic for
--enabledand--lockedis broken.v.IsSet()returns true for Viper-managed defaults (set viav.SetDefault()inBindFlagsToViper), not just for user-provided values. This meansenabledPtrandlockedPtrwill always be set to their default values (false) even when the user doesn't provide the flags, breaking the intended "nil = show all" semantics.Use Cobra's
flag.Changedproperty instead to properly distinguish user-set from default values:if f := cmd.Flags().Lookup("enabled"); f != nil && f.Changed { val := v.GetBool("enabled") enabledPtr = &val }This pattern is already used consistently throughout the Atmos codebase (e.g.,
cmd/root.go,cmd/cmd_flag_utils.go) and correctly supports tri-state logic.Also applies to: lines 59-82, 231-263
🧹 Nitpick comments (2)
internal/exec/stack_processor_cache.go (1)
185-200: Consider idiomatic type assertion for cache retrieval.Line 190 uses
fmt.Sprintf("%s", existingContent)to convert cached content. Since you store[]byteat line 197, a type assertion would be more idiomatic:existingContent, found := getFileContentSyncMap.Load(filePath) if found && existingContent != nil { - return fmt.Sprintf("%s", existingContent), nil + if contentBytes, ok := existingContent.([]byte); ok { + return string(contentBytes), nil + } }cmd/list/components.go (1)
312-371: Column flag parsing is reasonable; optional improvement to avoidstrings.Title/ nolint
parseColumnsFlag/parseColumnSpeccovers the expected cases:
- Bare field names like
component→Name: "Component", Value: "{{ .component }}".Name=templateforms likeComponent={{ .component }}with a safety wrapper when the value doesn’t already contain{{.One minor nit is the use of
strings.Titleplus astaticchecknolint. If you ever want to drop the nolint and modernize, you could replace it with a tiny ASCII‑only title‑casing helper (e.g., uppercasing just the first rune and leaving the rest as-is) and keep the behavior identical for column headers.Totally optional; current code is fine functionally.
📜 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/list/components.go(1 hunks)cmd/list/components_test.go(2 hunks)cmd/list/stacks.go(1 hunks)internal/exec/stack_processor_cache.go(1 hunks)internal/exec/stack_processor_cache_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/exec/stack_processor_cache_test.go
- cmd/list/components_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: 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/stack_processor_cache.gocmd/list/stacks.gocmd/list/components.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/list/stacks.gocmd/list/components.go
🧠 Learnings (60)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: 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: osterman
Repo: cloudposse/atmos PR: 1036
File: cmd/list_settings.go:46-50
Timestamp: 2025-02-20T13:57:36.326Z
Learning: The max-columns flag in Atmos list commands supports -1 for unlimited width, which bypasses terminal width restrictions and may require horizontal scrolling. The default is 10 columns if not specified.
📚 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_cache.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_cache.gocmd/list/stacks.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:
internal/exec/stack_processor_cache.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_cache.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_cache.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_cache.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_cache.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:
internal/exec/stack_processor_cache.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:
internal/exec/stack_processor_cache.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
internal/exec/stack_processor_cache.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/stack_processor_cache.gocmd/list/stacks.gocmd/list/components.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/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.gocmd/list/stacks.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: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
internal/exec/stack_processor_cache.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:
cmd/list/stacks.gocmd/list/components.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/list/stacks.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/list/stacks.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:
cmd/list/stacks.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/list/stacks.gocmd/list/components.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/list/stacks.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/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.go
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
cmd/list/stacks.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/list/stacks.gocmd/list/components.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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 : Never use `fmt.Fprintf(os.Stdout/Stderr, ...)`, `fmt.Println()`, or direct stream access for I/O. Always use `data.*` for pipeable output or `ui.*` for user-facing messages
Applied to files:
cmd/list/stacks.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 : For output: Pipeable data → `data.*` (stdout), Plain UI → `ui.Write*()` (stderr), Status messages → `ui.Success/Error/Warning/Info()` (stderr), Formatted docs → `ui.Markdown*()`. Use `data.Write()`, `data.Writef()`, `data.Writeln()`, `data.WriteJSON()`, `data.WriteYAML()` for data output
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.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/io/**/*.go : Use I/O layer (`pkg/io/`) for stream access (stdout/stderr/stdin), terminal capabilities, masking. Use UI layer (`pkg/ui/`) for formatting (colors, styles, markdown rendering). Never use `fmt.Fprintf(os.Stdout/Stderr, ...)`, `fmt.Println()`, or direct stream access
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.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/list/stacks.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/**/*_test.go : NEVER use pipe redirection (`2>&1`, `| head`, `| tail`) when running tests. Piping breaks TTY detection → ASCII fallback → wrong snapshots. NEVER manually edit golden snapshot files - always use `-regenerate-snapshots` flag. Snapshots capture exact output including invisible formatting (lipgloss padding, ANSI codes, trailing whitespace)
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.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 : ALWAYS ask for user approval before adding nolint comments. Prefer refactoring over nolint - only use nolint as last resort with explicit user permission. Exception for bubbletea models: `//nolint:gocritic // bubbletea models must be passed by value`. Exception for intentional subprocess calls: `//nolint:gosec // intentional subprocess call`. NEVER add nolint for gocognit, cyclomatic complexity, magic numbers, or nestif - refactor instead
Applied to files:
cmd/list/stacks.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 : Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`
Applied to files:
cmd/list/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 **/*.go : Document complex logic with inline comments in Go code
Applied to files:
cmd/list/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 **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
cmd/list/stacks.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.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:
cmd/list/stacks.gocmd/list/components.go
📚 Learning: 2024-12-08T14:26:16.972Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 828
File: pkg/schema/schema.go:98-100
Timestamp: 2024-12-08T14:26:16.972Z
Learning: The `ListConfig` columns array in the `Components` struct can be empty.
Applied to files:
cmd/list/components.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/list/components.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:
cmd/list/components.go
📚 Learning: 2025-06-07T19:28:21.289Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: cmd/describe_affected.go:0-0
Timestamp: 2025-06-07T19:28:21.289Z
Learning: In the Atmos codebase, using panic for unsupported flag types in flag processing functions like setDescribeAffectedFlagValueInCliArgs is the expected behavior rather than returning errors. This pattern is preferred for developer errors when unsupported types are added to the flagsKeyValue map.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
cmd/list/components.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/list/components.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 : Use `flags.NewStandardParser()` for command-specific flag parsing. NEVER call `viper.BindEnv()` or `viper.BindPFlag()` directly - use the unified `pkg/flags/` infrastructure instead. Register flags with Cobra command and bind to Viper for precedence handling
Applied to files:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-02-09T14:38:53.443Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/cmd_utils.go:0-0
Timestamp: 2025-02-09T14:38:53.443Z
Learning: Error handling for RegisterFlagCompletionFunc in AddStackCompletion is not required as the errors are non-critical for tab completion functionality.
Applied to files:
cmd/list/components.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:
cmd/list/components.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer. This is necessary for proper handling of recursive directory matching when using the gobwas/glob library.
Applied to files:
cmd/list/components.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/list/components.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/list/components.go
🧬 Code graph analysis (2)
internal/exec/stack_processor_cache.go (4)
pkg/schema/schema.go (1)
BaseComponentConfig(845-860)pkg/merge/merge.go (1)
DeepCopyMap(29-45)pkg/perf/perf.go (1)
Track(121-138)pkg/store/store.go (1)
Store(6-10)
cmd/list/components.go (8)
pkg/flags/global_registry.go (1)
ParseGlobalFlags(33-74)internal/exec/cli_utils.go (1)
ProcessCommandLineArgs(80-201)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/list/extract/components.go (1)
Components(18-47)pkg/list/column/column.go (2)
NewSelector(52-79)Config(13-17)pkg/list/renderer/renderer.go (1)
New(31-46)pkg/list/output/output.go (1)
New(14-16)pkg/list/sort/sort.go (4)
Sorter(35-39)NewSorter(48-54)Ascending(17-17)ParseSortSpec(195-237)
⏰ 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). (40)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
🔇 Additional comments (7)
internal/exec/stack_processor_cache.go (2)
156-162: Add perf.Track wrapper.Per coding guidelines, all public functions must include
defer perf.Track(nil, "exec.ClearBaseComponentConfigCache")()followed by a blank line.func ClearBaseComponentConfigCache() { + defer perf.Track(nil, "exec.ClearBaseComponentConfigCache")() + baseComponentConfigCacheMu.Lock() defer baseComponentConfigCacheMu.Unlock() baseComponentConfigCache = make(map[string]*schema.BaseComponentConfig)⛔ Skipped due to learnings
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 paramLearnt 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.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.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.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.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.Learnt from: Listener430 Repo: cloudposse/atmos PR: 844 File: pkg/config/cache.go:69-97 Timestamp: 2024-12-13T16:51:37.868Z Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.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.
133-143: Add perf.Track to ClearBaseComponentConfigCache and ClearJsonSchemaCache.Lines 158 and 166: These public functions are missing
defer perf.Track(nil, "exec.ClearBaseComponentConfigCache")()anddefer perf.Track(nil, "exec.ClearJsonSchemaCache")()respectively. Follow the pattern used in ClearFileContentCache (line 175).Use idiomatic string conversion at line 190.
Replace
fmt.Sprintf("%s", existingContent)withstring(existingContent). The value is already[]byte; direct conversion is more efficient and idiomatic.The jsonschema.Schema thread-safety assumption is valid—the library explicitly guarantees thread-safe validation on compiled schemas, supporting the compile-once-reuse-many pattern.
⛔ Skipped due to learnings
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.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.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.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.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.Learnt from: Listener430 Repo: cloudposse/atmos PR: 844 File: pkg/config/cache.go:69-97 Timestamp: 2024-12-13T16:51:37.868Z Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.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.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 duplicateLearnt 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 documentationLearnt 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.cmd/list/components.go (1)
88-115: Column completion and column resolution correctly respect empty config and defaultsThe
columnsCompletionForComponents+getComponentColumnspath looks solid:
- Completion pulls
components.list.columnsfrom atmos.yaml viaProcessCommandLineArgs+InitCliConfig, then surfaces just the configured names.getComponentColumnscorrectly prefers the--columnsflag, falls back to configuredComponents.List.Columns, and only then uses sane defaults (Component/Stack/Type).- If
Components.List.Columnsis an empty array (per schema learning that this is allowed), you correctly hit the default column set instead of passing an empty config slice intocolumn.NewSelector, which would error. Based on learnings.No changes needed here.
Also applies to: 268-296
cmd/list/stacks.go (4)
32-71: Stacks command wiring, parsing, and dispatch look consistent and cleanNice job on the stacks command refactor:
StacksOptionscleanly extendsglobal.Flagswith the new knobs (Component/Format/Columns/Sort/Provenance).- You’re using
NewListParserwith the expected wrappers, thenBindToViperininit()andBindFlagsToViperinRunE, matching the StandardParser pattern from the guidelines.RunEjust does config checking, flag parsing, options assembly, and delegates tolistStacksWithOptions, keeping CLI glue thin as requested.listStacksWithOptionsthen orchestrates init → extract → render, which mirrors the components pipeline and keeps the public surface tidy.This all aligns well with the repo’s CLI/flags patterns.
Also applies to: 102-124, 126-158
160-203: Tree/provenance handling and component filtering are now cohesiveThe new tree/provenance path for
list stackshangs together nicely:
validateProvenanceFlagcatches the explicit misuse (--provenancewith a non‑tree--format) before doing heavier work.initStacksConfignow usesProcessCommandLineArgs("list", cmd, args, nil)andInitCliConfigso base‑path/config flags and profiles are honored, then resolvesopts.FormatfromStacks.List.Formatwhen not set via CLI, and re‑validates provenance against the effective format. This fixes the earlier “config default format=tree + --provenance” edge case.renderStacksTreeFormattogglesTrackProvenance, clears caches, re‑runsExecuteDescribeStacks, and then renders viaformat.RenderStacksTreetodata.Writeln, which respects the I/O abstraction guidelines.resolveAndFilterImportTrees+buildAllowedStacksSetcorrectly restrict tree output to the already‑filteredstacksslice, so--componentaffects both table and tree views consistently.Overall this addresses the earlier provenance/tree concerns cleanly.
Also applies to: 299-333, 335-366, 368-379
73-100: Stacks column completion and defaults mirror components behavior correctlyFor stacks:
columnsCompletionForStacksusesProcessCommandLineArgs+InitCliConfig(withprocessStacks=false) to readstacks.list.columnsand returns just the configured names for shell completion.getStackColumnsgives precedence to the--columnsflag, then toStacks.List.Columns, and finally to sensible defaults:
- When
hasComponentis true, it shows bothStackandComponent.- Otherwise, it shows only
Stack.If
Stacks.List.Columnsis empty in config, you fall back to these defaults rather than passing an empty slice tocolumn.NewSelector, matching the components side and handling the “columns may be empty” pattern gracefully. Based on learnings.Looks good as-is.
Also applies to: 262-297
204-250: Stacks table rendering pipeline lines up with shared list infrastructure
renderStacksTableandbuildStackSortersuse the shared list infrastructure as intended:
- Filters currently come only from extraction (component) with
buildStackFiltersreserved for future CLI filters.- Columns are resolved via
getStackColumns, sharingparseColumnsFlagwith components for consistent flag syntax.- Sorters default to
"Stack"ascending, andParseSortSpecnow title‑cases the column name part so--sort stack:ascmatches that default header casing.- Renderer is constructed with filters, selector, sorters, and
format.Format(opts.Format)plus empty delimiter, and output is delegated to the list renderer/output packages.No issues here; the table path is nicely aligned with the rest of the list pipeline.
Also applies to: 381-392
|
@coderabbitai full review please |
|
@aknysh: I'll conduct a full review of the PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/exec/stack_processor_utils.go (1)
461-465: GuardatmosConfig.TrackProvenancewith a nil check to avoid panicsIn
processYAMLConfigFileWithContextInternal, a few conditionals dereferenceatmosConfigwithout checking for nil:
- Line 461:
if atmosConfig.TrackProvenance && mergeContext != nil && len(positions) > 0 { ... }- Line 588:
if atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() && len(importStructs) > 0 { ... }- Line 827:
if atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() { ... }Other places (e.g., line 715) already use
atmosConfig != nil && atmosConfig.TrackProvenance, so this is just an inconsistency. If any caller ever passes a nilatmosConfig, these branches will panic.A small guard keeps this future-proof and consistent:
- if atmosConfig.TrackProvenance && mergeContext != nil && len(positions) > 0 { + if atmosConfig != nil && atmosConfig.TrackProvenance && mergeContext != nil && len(positions) > 0 { @@ - if atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() && len(importStructs) > 0 { + if atmosConfig != nil && atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() && len(importStructs) > 0 { @@ - if atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() { + if atmosConfig != nil && atmosConfig.TrackProvenance && mergeContext != nil && mergeContext.IsProvenanceEnabled() {Also applies to: 588-615, 827-846
cmd/list/settings_test.go (1)
188-212: Align test name/comment withdisplayNoSettingsFoundMessageand fix comment punctuation.The test now exercises
displayNoSettingsFoundMessage, but both the test name (TestLogNoSettingsFoundMessage) and the preceding comment still reference the oldlog...name, and the inline comment at Line 206 is missing a trailing period. Renaming the test/comment to matchdisplayNoSettingsFoundMessageand ending the comment with a period will keep things clear and satisfy thegodotlinter.For example:
-// TestLogNoSettingsFoundMessage tests the logNoSettingsFoundMessage function. -func TestLogNoSettingsFoundMessage(t *testing.T) { +// TestDisplayNoSettingsFoundMessage tests the displayNoSettingsFoundMessage function. +func TestDisplayNoSettingsFoundMessage(t *testing.T) { @@ - // This function only displays UI output, so we just verify it doesn't panic + // This function only displays UI output, so we just verify it doesn't panic.pkg/list/format/formatter.go (1)
10-18: KeepFormatTreehandling consistent between validation and formatter construction.Right now
ValidateFormattreats"tree"as valid and includes it in theValidlist, butNewFormatterhas noFormatTreecase and itsInvalidFormatError.Validslice still omits"tree". That meansValidateFormat("tree")can succeed whileNewFormatter(FormatTree)still errors and reports a Valid list that disagrees.Two options to tighten this up:
If tree output is meant to use the same
Formatterinterface:
- Add a
case FormatTree:branch inNewFormatterthat returns an appropriate tree formatter.- Include
"tree"in theValidslice inside theInvalidFormatErrorthere as well.If tree is intentionally handled via a completely separate rendering path:
- Consider excluding
FormatTreefromValidateFormatand/or documenting thatFormatTreeis not supported byNewFormatter, so callers don’t attempt to construct a generic formatter for it.Either way, making
ValidateFormat,NewFormatter, and theInvalidFormatError.Validlists agree will avoid confusing behavior for callers.Also applies to: 60-76, 79-90
♻️ Duplicate comments (1)
internal/exec/stack_processor_cache.go (1)
156-170: Perf tracking on cache-clear helpers is inconsistent with the rest of this module.
ClearBaseComponentConfigCacheandClearJsonSchemaCacheare public, do real work, and sit next to other exported functions that are instrumented withperf.Track(e.g.,GetFileContent,ClearFileContentCache,getCachedBaseComponentConfig). For consistency with the repo’s profiling policy and prior feedback onClearJsonSchemaCache, consider adding:func ClearBaseComponentConfigCache() { - baseComponentConfigCacheMu.Lock() + defer perf.Track(nil, "exec.ClearBaseComponentConfigCache")() + + baseComponentConfigCacheMu.Lock() defer baseComponentConfigCacheMu.Unlock() baseComponentConfigCache = make(map[string]*schema.BaseComponentConfig) }and similarly for
ClearJsonSchemaCache.
🧹 Nitpick comments (35)
cmd/root.go (2)
411-414: Clarify color-profile precedence to avoid surprises when NO_COLOR / flags are set.Right now
configureEarlyColorProfile(cmd)can set an explicit lipgloss profile based onNO_COLOR,--no-color, or--force-color, and then this block unconditionally overwrites it withterminal.New().ColorProfile()viaconvertToTermenvProfile. Ifpkg/terminaldoesn’t already fold in those flags/env vars, this could re-enable colors even when the user asked for none, or change a forced profile unexpectedly.Consider either:
- Letting
pkg/terminalown the full precedence (and dropping the extralipgloss.SetColorProfilecalls inconfigureEarlyColorProfile), or- Only calling this block when no explicit NO_COLOR / force-color preference has been applied, and documenting that precedence in comments.
That keeps color behavior predictable and makes it easier to reason about which layer “wins.”
1303-1320: Tidy up convertToTermenvProfile comments and nolint.The doc comment block above this function still talks about
displayPerformanceHeatmap, and the//nolint:unparamrefers to a now-nonexistentcmdparameter whileprofileis actually used, so the suppression is redundant.You could simplify to something like:
-// displayPerformanceHeatmap shows the performance heatmap visualization. -// -// ConvertToTermenvProfile converts our terminal.ColorProfile to termenv.Profile. -// -//nolint:unparam // cmd parameter reserved for future use -func convertToTermenvProfile(profile terminal.ColorProfile) termenv.Profile { +// convertToTermenvProfile converts a terminal.ColorProfile to a termenv.Profile. +func convertToTermenvProfile(profile terminal.ColorProfile) termenv.Profile {and, if you still want a doc comment on
displayPerformanceHeatmap, move its comment block down to sit directly above that function.The mapping logic itself (None→Ascii, 16→ANSI, 256→ANSI256, True→TrueColor, default→Ascii) looks good.
.claude/agents/tui-list.md (1)
379-397: Align tree-format example with data/ui output conventionsThe tree-format example still shows
fmt.Println(output), while the repo’s I/O rules route list output throughpkg/data/pkg/ui. To avoid new code copying this pattern, consider updating the snippet to use the data channel instead, e.g.:- output := format.RenderStacksTree(importTrees, opts.Provenance) - fmt.Println(output) + output := format.RenderStacksTree(importTrees, opts.Provenance) + _ = data.Write(output)pkg/list/extract/workflows.go (1)
17-51: Workflow extraction behavior looks solid; consider refactoring to drop broad nolintThe file/fileFilter and directory-based extraction paths, error wrapping with
ErrParseFile/ErrWorkflowDirectoryDoesNotExist, and perf tracking all look correct and match the tests.The only concern is the broad
//nolint:gocognit,nestif,revive,funlenonWorkflows. Repo guidance asks to avoid adding nolint for cognitive complexity/nesting and instead split logic into helpers (e.g., one helper for “single file path”, another for “scan workflows dir + per-file parse”) so the main entry point stays small and the suppressions aren’t needed.Also applies to: 53-95, 97-120
pkg/list/extract/workflows_test.go (1)
195-205: Avoid string-based error assertions on error messagesThese tests already assert the correct sentinel via
assert.ErrorIs, which is the preferred pattern here. The additionalassert.Contains(t, err.Error(), "...")checks go against the repo guideline to avoiderr.Error()string inspection in tests and make the tests more brittle if wording changes.You could safely drop the
Containsassertions and rely onErrorIs, or, if you need to distinguish subcases, add structured error context (fields or wrapping) and assert on that instead.Also applies to: 208-218
cmd/list/values_test.go (1)
292-325: Rename test to reflect the new display helper rather than loggingThe body now calls
displayNoValuesFoundMessage, and the inline comment mentions “displays UI output”, but the test name/comment still refer to “LogNoValuesFoundMessage”. To keep things clear for future readers, consider renaming the test and its doc comment, e.g.:-// TestLogNoValuesFoundMessage tests that the correct log message is generated. -func TestLogNoValuesFoundMessage(t *testing.T) { +// TestDisplayNoValuesFoundMessage ensures the UI helper does not panic. +func TestDisplayNoValuesFoundMessage(t *testing.T) {pkg/list/format/tree_utils.go (1)
44-61: Consider usingstrings.Builderand documenting ANSI assumptions instripANSIThe manual state machine is fine for SGR sequences (
\x1b...m), but:
- Repeated
result += string(r)can get quadratic for larger outputs.- The implementation silently ignores non‑
m‑terminated escape sequences.You might switch to a
strings.Builderand add a short comment that this is intentionally scoped to SGR “...m” sequences produced by lipgloss, so future changes don’t assume it’s a general ANSI stripper.cmd/list/stacks_test.go (1)
349-417: Avoid string-matching provenance errors; prefer sentinel errors andassert.ErrorIs
TestListStacksWithOptions_ProvenanceValidationchecks for provenance failures viaassert.Contains(t, err.Error(), "--provenance")/assert.NotContains, which couples tests to error text and goes against the repo guidance to useassert.ErrorIswith sentinel errors for our own error types.Consider:
- Introducing a dedicated sentinel (e.g.
errors.ErrInvalidProvenanceFormat) in the errors package and returning it from the early validation, then- Switching tests to
require.ErrorIs(t, err, errors.ErrInvalidProvenanceFormat)and only fall back to string matching for truly third‑party errors.This will make the tests more robust to wording tweaks while still catching provenance‑specific failures.
As per coding guidelines, tests should use
assert.ErrorIsfor our own errors rather than string matching.docs/list-flag-wrappers.md (1)
205-241: Clarify binding pattern (BindToVipervsBindFlagsToViper) and referenced file locationsThe guide currently shows two slightly different patterns:
- In the usage examples,
NewListParser(...)is followed ininit()byparser.RegisterFlags(cmd)and_ = parser.BindToViper(viper.GetViper()).- In the migration section,
RunEusescomponentsParser.BindFlagsToViper(cmd, v)as the binding step.Given the existing StandardParser pattern in this repo, it would help to explicitly call out which method is the recommended one for list commands (and when to use it), so contributors don’t cargo‑cult the less preferred style.
Also, the “Files Created” section lists
cmd/list/FLAG_WRAPPERS.mdandcmd/list/IMPLEMENTATION_GUIDE.md, while the docs in this PR live underdocs/. It’d be good to align these paths (or note both, if the cmd/ copies are still authoritative) so newcomers know where to look.Based on learnings, this repo standardizes on the StandardParser + Viper pattern, so having one clear, consistent example will reduce confusion.
Also applies to: 263-304, 357-364
pkg/list/format/tree_instances_test.go (1)
10-129: Tree instance rendering tests look good; spacer detection could be slightly less brittleThese tests nicely exercise header presence, stack/component inclusion, empty‑input messaging, and spacer behavior for one vs multiple stacks, and using
stripANSIkeeps them independent of color decisions.One minor thought: both spacer tests assume spacer lines are exactly
"│". If the tree glyphs ever change, these tests will break even though the high‑level behavior is still correct. You could factor the spacer glyph into a shared const in the format package (or reuse whatever the renderer uses) and reference that here to keep the tests aligned with the implementation.docs/list-implementation-guide.md (1)
205-279: Make examples and references repo-agnostic (binding pattern and paths)The implementation guide is very helpful, but a couple of details might trip up other contributors:
- Binding: Like the other doc, some examples use
parser.BindToViper(viper.GetViper())ininit(), while the migration section showsBindFlagsToViper(cmd, v)inRunE. It’d be good to state explicitly which pattern list commands should follow so folks don’t cargo‑cult the less preferred one.- Paths: The “Files Created” and “Additional Resources” sections reference concrete local paths like
/Users/erik/...andcmd/list/FLAG_WRAPPERS.md, whereas the docs in this PR are underdocs/. Consider switching to repo‑relative paths (e.g.,docs/prd/list-commands-ui-overhaul.md,docs/list-flag-wrappers.md) or removing the local-machine paths to keep the guide portable.Tightening those up will make this an even better on‑ramp for future contributors.
Also applies to: 357-365, 396-401
pkg/list/extract/metadata_test.go (1)
290-332: AlignTestGetStatusIndicatornames with what’s actually assertedThe table names talk about “shows green/red/gray”, but the assertions only check that the returned string contains the
●character and is non‑empty, without validating any color/ANSI aspects. That’s probably the right trade‑off for stability, but the names may mislead future readers.Either:
- Rename the cases to describe enabled/locked combinations without color (e.g., “enabled and unlocked returns non-empty indicator”), or
- If you do want to assert color, strip ANSI and/or look for specific SGR sequences, with a comment that ties it to the current theme.
Right now the behavior is fine; this is just about keeping the test intent crystal‑clear.
internal/exec/stack_processor_template_test.go (1)
153-170: Consider a small helper to avoid repeatednolint:dogsledmulti-assignmentsUpdating the tests to handle the extra
*MergeContextreturn fromProcessYAMLConfigFileWithContextis correct, and it makes sense that these cases only care about the primary result and error.To keep things a bit cleaner (and avoid relying on
//nolint:dogsledin multiple places), you might introduce a tiny test-only helper:func processYAMLForTest( atmosConfig *schema.AtmosConfiguration, basePath, filePath string, ctx map[string]any, skipTemplatesProcessingInImports, ignoreMissingTemplateValues bool, ) (map[string]any, error) { result, _, _, _, _, _, _, _, err := ProcessYAMLConfigFileWithContext( atmosConfig, basePath, filePath, map[string]map[string]any{}, ctx, false, // ignoreMissingFiles skipTemplatesProcessingInImports, ignoreMissingTemplateValues, false, // skipIfMissing nil, nil, nil, nil, "", // atmosManifestJsonSchemaFilePath nil, // mergeContext ) return result, err }Then the tests can just call
processYAMLForTest(...)without wide assignments or nolint comments.Not mandatory, but it would match the repo’s general preference to minimize
nolintusage where simple refactors suffice.As per coding guidelines,
nolintis meant as a last resort; a small helper here can keep the tests tidy without disabling the linter.Also applies to: 320-336, 389-406, 422-438
cmd/list/settings.go (1)
112-159: Nice NoValuesFound handling; consider surfacing more context in the message.The new
displayNoSettingsFoundMessageand theerrors.Ascheck for*listerrors.NoValuesFoundErrormake the UX much nicer here (UI message + zero exit code instead of a hard error), and the fallthrough for other errors is clean.If you want to give users a bit more guidance when a
Querywas used (e.g.,--queryor other filters), you could optionally incorporatenoValuesErr.Queryinto the info message so it’s obvious which filter yielded no results; otherwise the current behavior is perfectly fine to ship. As per coding guidelines, this keeps user‑facing feedback in theuilayer instead of the logger.cmd/list/flag_wrappers_test.go (1)
377-414:TestFlagEnvironmentVariableBindingisn’t actually asserting env binding (andenvVarNameis unused).Right now the table includes
envVarNamebut the test only checks thatBindToViperdoesn’t error; it never sets an env var or asserts that Viper sees it. That’s a bit misleading and misses coverage of theWithEnvVarswiring.Two options you might consider:
- Use
envVarName: in each subtest, callt.Setenv(tt.envVarName, "test"), then afterBindToViper(v)assert thatv.GetString(tt.flagName)(or the appropriate getter) resolves to"test".- Or, if you want to keep env behavior as an implementation detail of
flags.StandardParser, dropenvVarNamefrom the table to avoid suggesting that it’s verified here.Either way, tightening this test will make the env‑binding behavior less surprising and better aligned with the CLI precedence rules. As per coding guidelines, env precedence is important to validate.
docs/prd/list-commands-configurable-output.md (1)
55-1511: Add languages to bare fenced code blocks to satisfy markdownlint.markdownlint is flagging a handful of
blocks without a language (e.g., some of the template‑variable and error‑message examples around the “Template Variable Reference”, error‑message, and risks sections). To keep the docs pipeline happy, it’s worth tagging those with something like `yaml`, `bash`, or `text` as appropriate (e.g.,yaml for config snippets, ```text for raw error output). The rest of the PRD structure looks solid.pkg/list/extract/components_test.go (1)
11-245: Components extraction tests look solidNice coverage of happy paths, default metadata, and all the error sentinels (nil map, invalid stacks, no components, etc.). The tests are behavior-focused and use testify idiomatically. Only tiny nit: if
godotruns on tests too, you may want to add a trailing period to// vpc, eks, ingress, vpcso all comments terminate with periods.pkg/list/format/tree_instances.go (1)
132-175: Remove duplicatecomponentSpacerMarkerdefinitionYou already have
componentSpacerMarkeras a package‑level constant; re‑declaring it insidebuildStackNodeWithComponentsis redundant and risks drift if one changes:- const componentSpacerMarker = "<<<COMPONENT_SPACER>>>" - // Add component children with spacers between them. for i, componentName := range componentNames {Using the top‑level
componentSpacerMarkereverywhere keeps things simpler and avoids subtle bugs later if the marker text ever changes.pkg/list/importresolver/provenance_test.go (1)
363-415: Provenance/import resolver tests cover the edge cases wellThis suite does a good job exercising empty chains, circular refs, caching, deep recursion, and large import lists, and the move to
t.TempDir/filepath.Joinin the newer tests should keep things stable across platforms. If you want, you could later switch the small deep‑recursion helper fromrune('0'+i)tostrconv.Itoa(i)for consistency with the newer tests, but that’s purely cosmetic at this point.pkg/list/importresolver/resolver.go (1)
154-193: Optionally use the cache inbuildImportTreeto avoid re-reading filesRight now
buildImportTreealways callsreadImportsFromFile(importFilePath)and only then writes tocache[importFilePath]. If the same import file is referenced multiple times (across stacks or branches), you could avoid repeated disk reads by consulting the cache first:func buildImportTree(importPath string, atmosConfig *schema.AtmosConfiguration, cache map[string][]string, visited map[string]bool) *tree.ImportNode { @@ - // Read imports from this file. - childImports, err := readImportsFromFile(importFilePath) - if err != nil { - // If we can't read the file, just return the node without children. - return node - } - - // Cache the imports. - cache[importFilePath] = childImports + // Read imports from this file, using cache if available. + childImports, ok := cache[importFilePath] + if !ok { + var err error + childImports, err = readImportsFromFile(importFilePath) + if err != nil { + // If we can't read the file, just return the node without children. + return node + } + cache[importFilePath] = childImports + }Not required for correctness, but it keeps IO bounded when many stacks share the same imported catalogs.
cmd/list/instances.go (1)
72-97: Missing perf.Track in completion function.Per coding guidelines, public functions should have perf tracking.
columnsCompletionForInstanceslacks thedefer perf.Track(nil, "list.instances.columnsCompletionForInstances")()call that similar functions incmd/list/components.gohave.func columnsCompletionForInstances(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + defer perf.Track(nil, "list.instances.columnsCompletionForInstances")() + // Load atmos configuration. configAndStacksInfo, err := e.ProcessCommandLineArgs("list", cmd, args, nil)docs/prd/provenance-import-resolution.md (1)
294-331: Add language specifier to the ASCII diagram code block.The static analysis (MD040) flags that fenced code blocks should have a language specified. For ASCII diagrams, use
textorplaintext.-``` +```text ┌─────────────────────────────────────────────────┐ │ ExecuteDescribeStacks() or describe component │pkg/list/format/table_test.go (1)
291-339: Markdown rendering tests could use a negative case.The tests verify content is present but don't verify that markdown syntax is actually stripped/rendered. Consider adding an assertion that
**or*markers are removed from output.{ name: "Bold text", input: "**Important** configuration", contains: "Important", }, + { + name: "Bold markers removed", + input: "**Bold**", + contains: "Bold", + // Could add: notContains: "**" + },pkg/list/importresolver/resolver_test.go (1)
296-313: Consider usingstrconv.Itoafor clearer digit conversion.The
string(rune('0'+i))pattern works for single digits but is less obvious thanstrconv.Itoa(i)orfmt.Sprintf("%d", i). The current approach also has a hidden assumption thatistays single-digit.+import "strconv" + for i := 1; i <= 5; i++ { var content string if i < 5 { content = ` imports: - - deep/level` + string(rune('0'+i+1)) + ` + - deep/level` + strconv.Itoa(i+1) + ` vars: - level: ` + string(rune('0'+i)) + level: ` + strconv.Itoa(i) } else { content = ` vars: level: 5 ` } - path := filepath.Join(deepDir, "level"+string(rune('0'+i))+".yaml") + path := filepath.Join(deepDir, "level"+strconv.Itoa(i)+".yaml")pkg/list/column/column.go (1)
147-168: Column order follows config definition, not--columnsflag order.
getSelectedConfigsfilters configs in their original definition order, not the order specified in--columns. If a user passes--columns=B,Abut configs define A before B, they'll see A,B. This may be intentional for consistency, but worth documenting if so.To respect
--columnsflag order instead:func (s *Selector) getSelectedConfigs() []Config { if len(s.selected) == 0 { return s.configs } - // Build map for quick lookup - selectedMap := make(map[string]bool) - for _, name := range s.selected { - selectedMap[name] = true - } - - // Filter configs to selected columns in original order - var configs []Config - for _, cfg := range s.configs { - if selectedMap[cfg.Name] { - configs = append(configs, cfg) - } - } + // Build map for quick lookup + configMap := make(map[string]Config) + for _, cfg := range s.configs { + configMap[cfg.Name] = cfg + } + + // Return configs in selected order + var configs []Config + for _, name := range s.selected { + if cfg, ok := configMap[name]; ok { + configs = append(configs, cfg) + } + } return configs }pkg/list/list_instances.go (1)
308-308: Useui.Infoinstead ofPrintfMessageToTUI.Per coding guidelines, status messages should use
ui.Success/Error/Warning/Info()for stderr output.PrintfMessageToTUIis a legacy utility.- u.PrintfMessageToTUI("Successfully uploaded instances to Atmos Pro API.") + _ = ui.Info("Successfully uploaded instances to Atmos Pro API.")pkg/list/format/table.go (1)
514-520: Consider pre-compiling the whitespace regex.The regex
\s+is compiled on everyrenderInlineMarkdowncall. For tables with many Description cells, this adds overhead.+var whitespaceRegex = regexp.MustCompile(`\s+`) + func renderInlineMarkdown(content string) string { ... - singleLine = regexp.MustCompile(`\s+`).ReplaceAllString(singleLine, " ") + singleLine = whitespaceRegex.ReplaceAllString(singleLine, " ")cmd/list/vendor.go (1)
175-191: Stack flag filters on component field - document this.The
--stackflag filters on"component"since vendor rows don't have stack data. The help text says "Filter by stack pattern" which could confuse users.Consider either:
- Adding a dedicated
--componentflag for vendor and removingWithStackFlag, or- Updating the help text in
WithStackFlagwhen used for vendor to clarify it filters component names.This is a UX clarity issue, not a bug.
cmd/list/flag_wrappers.go (1)
53-75: Multiple columns flags are identical - consider consolidation.
WithInstancesColumnsFlag,WithMetadataColumnsFlag,WithStacksColumnsFlag,WithWorkflowsColumnsFlag, andWithVendorColumnsFlagall do the same thing. This duplication seems intentional for future divergence, but currently adds cognitive overhead.If these are expected to remain identical, consider a single
WithColumnsFlagthat all commands use. If they're placeholders for future command-specific behavior, add a comment noting that intent.cmd/list/components_test.go (2)
138-230: Several ComponentsOptions tests are essentially tautological.
TestComponentsOptions_AllFields(and the earlier simple options tests) only verify that struct fields equal the values they were initialized with, without exercising any behavior that consumesComponentsOptions. This drifts from the “test behavior, not implementation” guideline and adds maintenance cost without guarding real regressions. Consider either:
- Pivoting these cases to assert behavior of helpers that use
ComponentsOptions(e.g., filter/sorter builders), or- Dropping the purely structural cases and relying on behavior‑oriented tests like
TestBuildComponentFiltersinstead.
765-936: RenderComponents tests could assert observable behavior, not just absence of errors.
TestRenderComponents,TestRenderComponents_TriStateBoolFilters, andTestRenderComponents_TypeFiltercurrently only check error/no‑error, not that filtering/sorting actually change the rendered rows. If you want stronger coverage of the new list pipeline, you could capture the writer frompkg/io(initialized viainitTestIO) and assert on:
- Row counts after stack/tri‑state filters, and
- Ordering for
Sortspecs.Not urgent, but it would turn these into behavioral tests rather than smoke tests.
Also applies to: 938-1082
pkg/list/filter/filter.go (1)
119-161: Confirm BoolFilter’s “truthy” handling matches expectations.
BoolFilter.Applytreats any non‑bool value as a string and then:
- Interprets
"true","yes","1"astrue, and- Treats everything else (including
"false","no","0", or arbitrary strings) asfalse.That’s a reasonable heuristic, but it means e.g.
"false"or"no"will be included when filtering forValue == false. If upstream data ever encodes booleans as strings, this is behaviorally significant. If that’s not what you want, you may want a more explicit mapping (e.g. recognize"false","no","0"distinctly).internal/exec/stack_processor_cache.go (1)
185-200: Avoidfmt.Sprintfwhen reading from the file-content cache.In
GetFileContent, cached entries are stored as[]byte, but the fast path usesfmt.Sprintf("%s", existingContent)to convert them back to a string. This works because the dynamic type is[]byte, but it adds unnecessary formatting overhead.You can make this cheaper and clearer by asserting the type:
- existingContent, found := getFileContentSyncMap.Load(filePath) - if found && existingContent != nil { - return fmt.Sprintf("%s", existingContent), nil - } + existingContent, found := getFileContentSyncMap.Load(filePath) + if found { + if b, ok := existingContent.([]byte); ok { + return string(b), nil + } + }(Or store strings in the map and skip the cast entirely.)
pkg/list/importresolver/provenance.go (1)
240-271: Consider trimming unused helper and double-checking new nolints.
extractComponentsFromStackDataisn’t referenced anywhere in this file anymore; if it’s also unused across the repo, you could drop it to keep the importresolver surface minimal. If there are external callers, maybe add a brief doc comment showing how/where it’s intended to be used so it doesn’t look like leftover scaffolding.Also,
ResolveImportTreeFromProvenanceandfindStacksForFilePathcarry//nolint:gocognit,revive/funlensuppressions. The repo guidelines generally discourage new nolints for cognitive complexity/length; if maintainers are happy with these as intentional exceptions that’s fine, but otherwise the next round of cleanup might be to factor some of that logic into small, focused helpers and drop the nolints.Also applies to: 32-33, 106-107
cmd/list/stacks.go (1)
126-167: Provenance/format validation is strict—confirm this is the intended UX.With
validateProvenanceFlagplus the post-config check ininitStacksConfig, the effective rules are:
--provenanceand--format=tree→ allowed.--provenanceand any other explicit--format→ErrInvalidFlag.--provenancewith no--format, butstacks.list.format: treein config → allowed.--provenancewith no--formatand no config/default format → now treated as invalid, becauseopts.Formatremains""and fails the second check.That’s consistent and predictable, but it does mean
atmos list stacks --provenanceby itself is no longer accepted unless the format is set totreeelsewhere. If the intent was for--provenancealone to imply tree output, you’d instead want to defaultopts.Formattotreewhenopts.Provenanceis true and no format is set, and then validate.If you’re happy requiring users (or config) to set
format: treeexplicitly when using provenance, the current behavior is fine—just worth being aware of.Also applies to: 168-195
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/prd/auth-context-multi-identity.md (1)
479-506: Fix function signature inconsistency in PostAuthenticate example.The
PostAuthenticateexample signature (line 479) is missing theauthContext *schema.AuthContextparameter that was specified at line 318-320. This breaks the design pattern wherePostAuthenticatepopulates theauthContext.The function should receive
authContextas a parameter so it can populateauthContext.AWSduring authentication. Similarly, the call toSetAuthContextat line 492 should passstackInfo.AuthContext(from the received parameter) as the first argument.Apply this diff to fix the signature and call:
func (i *AssumeRoleIdentity) PostAuthenticate( ctx context.Context, + authContext *schema.AuthContext, stackInfo *schema.ConfigAndStacksInfo, providerName, identityName string, creds types.ICredentials, ) error { // ... existing credential setup ... if err := awsCloud.SetupFiles(providerName, identityName, creds); err != nil { return errors.Join(errUtils.ErrAwsAuth, err) } // NEW: Set auth context (single source of truth). - if err := awsCloud.SetAuthContext(stackInfo, providerName, identityName); err != nil { + if err := awsCloud.SetAuthContext(authContext, stackInfo, providerName, identityName, creds); err != nil { return errors.Join(errUtils.ErrAwsAuth, err) } // NEW: Derive environment variables from auth context. // This populates ComponentEnvSection from the auth context. if err := awsCloud.SetEnvironmentVariables(stackInfo); err != nil { return errors.Join(errUtils.ErrAwsAuth, err) } return nil } // Apply same pattern to PermissionSetIdentity and UserIdentity
♻️ Duplicate comments (2)
pkg/list/extract/stacks.go (1)
21-27: Optional: stabilize stack iteration order to avoid map nondeterminismRight now both functions iterate maps directly, so the order of
stacksdepends on Go’s map iteration order. If the downstream sort stage doesn’t fully normalize ordering (or if callers ever consume this slice without an explicit sort), consider sorting stack names first.Example refactor for
Stacks:-import ( - "fmt" - - errUtils "github.com/cloudposse/atmos/errors" - "github.com/cloudposse/atmos/pkg/perf" -) +import ( + "fmt" + "sort" + + errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/perf" +} @@ - for stackName := range stacksMap { - stack := map[string]any{ - "stack": stackName, - } - - stacks = append(stacks, stack) - } + stackNames := make([]string, 0, len(stacksMap)) + for name := range stacksMap { + stackNames = append(stackNames, name) + } + sort.Strings(stackNames) + + for _, stackName := range stackNames { + stacks = append(stacks, map[string]any{ + "stack": stackName, + }) + }And for
StacksForComponent:- for stackName, stackData := range stacksMap { - stackMap, ok := stackData.(map[string]any) + stackNames := make([]string, 0, len(stacksMap)) + for name := range stacksMap { + stackNames = append(stackNames, name) + } + sort.Strings(stackNames) + + for _, stackName := range stackNames { + stackData := stacksMap[stackName] + stackMap, ok := stackData.(map[string]any) @@ - if found { - stack := map[string]any{ - "stack": stackName, - "component": componentName, - } - stacks = append(stacks, stack) - } + if found { + stacks = append(stacks, map[string]any{ + "stack": stackName, + "component": componentName, + }) + }Only worth doing if a stable pre-sort here adds value beyond what the renderer/sort stage already guarantees.
Also applies to: 42-71
pkg/list/importresolver/resolver.go (1)
157-195: Make the import cache effective inbuildImportTree(currently write-only).
buildImportTreestores entries incache[importFilePath]but never reads from the cache, so repeated imports still re-read the same YAML files. Either drop the cache parameter or (preferably) use it to avoid duplicate IO.One way to wire the cache in without changing behavior:
- // Read imports from this file. - childImports, err := readImportsFromFile(importFilePath) - if err != nil { - // If we can't read the file, just return the node without children. - return node - } - - // Cache the imports. - cache[importFilePath] = childImports + // Reuse cached imports when available to avoid repeated IO. + childImports, ok := cache[importFilePath] + if !ok { + var err error + childImports, err = readImportsFromFile(importFilePath) + if err != nil { + // If we can't read the file, just return the node without children. + return node + } + cache[importFilePath] = childImports + }This keeps the external API the same while making the shared
importCachemeaningful.
🧹 Nitpick comments (5)
pkg/list/extract/metadata.go (1)
131-144: Consider usingstrings.Joinfor cleaner code.The manual comma concatenation works fine, but
strings.Join(values, ", ")is more idiomatic and concise.Apply this diff:
+import ( + "strings" + + "github.com/charmbracelet/lipgloss" + -// joinWithComma joins a string slice with comma separators. -func joinWithComma(values []string) string { - if len(values) == 0 { - return "" - } - - result := "" - for i, s := range values { - if i > 0 { - result += ", " - } - result += s - } - return result -} +// joinWithComma joins a string slice with comma separators. +func joinWithComma(values []string) string { + return strings.Join(values, ", ") +}pkg/list/extract/stacks.go (1)
15-17: Align stack-related errors with the central ErrorBuilder if these surface to usersThe raw use of
ErrStackNotFoundandErrNoStacksFoundis fine for internal plumbing, but if these errors are ultimately shown to users (e.g., throughatmos list ...), consider constructing them via the shared error builder so you can attach explanation, hints, and structured context likecomponentNamein a consistent way.Not a blocker here, but worth a follow-up pass once the list pipeline wiring is finalized.
As per coding guidelines, user-facing errors should normally flow through the central ErrorBuilder with context and hints.
Also applies to: 36-38, 73-75
pkg/list/importresolver/resolver.go (1)
81-107: Align comments with current behavior and fix punctuation.The implementation now just tries a few common path patterns; it no longer “search[es] through all stack files”, so the comment is misleading. Also, the comment on Line 85 lacks a trailing period, which will trip
godot.A minimal comment-only tweak:
-// Try to construct the file path from the stack name. -// Stack names follow pattern like "plat-ue2-prod" which maps to files in stacks/orgs/ - -// For now, we'll search through all stack files to find matches. -// This is not perfect but works for the common case. +// Try to construct the file path from the stack name. +// Stack names follow pattern like "plat-ue2-prod" which maps to files under stacks/orgs/. +// Heuristically try a few common path patterns; this is not perfect but covers common cases.cmd/list/components.go (1)
343-377: Optional: reconsiderstrings.Title+ nolint inparseColumnSpec.Using
strings.Titlewith//nolint:staticcheckis understandable to get simple ASCII title-casing for column names, but it does cut against the repo’s general “avoid nolint unless necessary” guidance and pulls in deprecated behavior. If you ever want to tighten this up, a small local helper (e.g., uppercasing the first rune) orgolang.org/x/text/caseswould avoid the nolint while keeping predictable output; tests already cover many edge cases.cmd/list/components_test.go (1)
259-265: AlignType-related tests with actual--typesemantics.In
TestBuildComponentFiltersandTestRenderComponents_TypeFilter, several cases useType: "terraform"/"helmfile"even though the--typeflag in this command is modeled around the metadatacomponent_type(real/abstract/all) rather than backend implementation. The tests still pass because they only assert counts or “no error,” but they no longer mirror realistic flag values.Not urgent, but it would be clearer to switch these cases to
real/abstract/allso the tables reflect actual CLI behavior. Based on learnings, this keeps tests anchored to real user inputs instead of unreachable states.Also applies to: 1034-1057
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/exec/stack_processor_cache.go (2)
94-99: Consider removing redundant return value.The inheritance chain is returned both as a separate
*[]stringand insidecopyConfig.ComponentInheritanceChain. If callers only need one access path, the signature could simplify to(*schema.BaseComponentConfig, bool).If both are intentional for caller convenience, this is fine as-is.
141-142: Variable shadows imported package name.Local
schemashadows the importedgithub.com/cloudposse/atmos/pkg/schemapackage. Consider renaming tocompiledSchemaorcachedSchemafor clarity.- schema, found := jsonSchemaCache[schemaPath] - return schema, found + compiledSchema, found := jsonSchemaCache[schemaPath] + return compiledSchema, foundcmd/list/components_test.go (2)
138-230: Avoid pointer identity checks for tri‑state booleans
TestComponentsOptions_AllFieldscurrently comparesEnabledandLockedvia pointer equality, which couples the test to how pointers are allocated rather than to the logical tri‑state behavior. Comparing nil/non‑nil plus the pointed‑to value would be more robust if the implementation ever copies or reconstructs those pointers.For example:
- assert.Equal(t, tc.expectedEnabled, tc.opts.Enabled) - assert.Equal(t, tc.expectedLocked, tc.opts.Locked) + if tc.expectedEnabled == nil { + assert.Nil(t, tc.opts.Enabled) + } else if assert.NotNil(t, tc.opts.Enabled) { + assert.Equal(t, *tc.expectedEnabled, *tc.opts.Enabled) + } + + if tc.expectedLocked == nil { + assert.Nil(t, tc.opts.Locked) + } else if assert.NotNil(t, tc.opts.Locked) { + assert.Equal(t, *tc.expectedLocked, *tc.opts.Locked) + }
765-931: renderComponents tests hit important paths; consider adding a tree/CSV case
TestRenderComponentsexercises empty/non‑empty inputs, table/json/yaml formats, invalid sort spec, stack filtering, config‑driven custom columns, and ascending/descending sorts, which gives strong coverage of the components render pipeline.Given this PR also introduces additional formats like CSV/TSV/tree, you might optionally add one or two extra cases (e.g.,
Format: "csv"andFormat: "tree"with a small component set) just to assert those paths don’t error at this wiring layer; detailed formatting is already covered elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cmd/list/components_test.go(2 hunks)internal/exec/stack_processor_cache.go(1 hunks)pkg/list/format/tree_instances.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Import organization: three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param
Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use//revive:disable:file-length-limit
Use sentinel errors fromerrors/errors.gowith ErrorBuilder for all user-facing errors. Useerrors.Is()for checking errors, never string comparison.WithCause()preserves underlying error messages,WithExplanation()adds context,WithHint()provides guidance,WithContext()adds key-value pairs
Usecontext.Contextas first parameter for: cancellation signals (prop...
Files:
pkg/list/format/tree_instances.gocmd/list/components_test.gointernal/exec/stack_processor_cache.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/list/format/tree_instances.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/list/components_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:
cmd/list/components_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
cmd.NewTestKit(t)for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Files:
cmd/list/components_test.go
🧠 Learnings (48)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-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/list/format/tree_instances.gointernal/exec/stack_processor_cache.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.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:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.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/list/format/tree_instances.gointernal/exec/stack_processor_cache.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.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/list/format/tree_instances.gointernal/exec/stack_processor_cache.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:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.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 : All comments must end with periods (enforced by `godot` linter)
Applied to files:
pkg/list/format/tree_instances.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
pkg/list/format/tree_instances.gointernal/exec/stack_processor_cache.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 : NEVER delete existing comments without a very strong reason. Comments document why/how/what/where. Preserve helpful comments, update to match code changes, refactor for clarity, add context when modifying. Only acceptable removals: factually incorrect, code removed, duplicates obvious code, outdated TODO completed
Applied to files:
pkg/list/format/tree_instances.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:
cmd/list/components_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:
cmd/list/components_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:
cmd/list/components_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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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/list/components_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : NEVER use `assert.Contains(err.Error(), ...)` for error checking. ALWAYS use `assert.ErrorIs()` instead. NEVER use string comparison: `err.Error() == "..."` or `strings.Contains(err.Error(), ...)`
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use `assert.ErrorIs(t, err, ErrSentinel)` for checking our own errors and stdlib errors. Only use string matching for third-party errors
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions - implement or remove. Avoid tautological tests. Make code testable using dependency injection to avoid `os.Exit`, `CheckErrorPrintAndExit`, external systems. Remove always-skipped tests - fix or delete. Table-driven tests must have real scenarios
Applied to files:
cmd/list/components_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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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:
cmd/list/components_test.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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/**/*_test.go : NEVER use pipe redirection (`2>&1`, `| head`, `| tail`) when running tests. Piping breaks TTY detection → ASCII fallback → wrong snapshots. NEVER manually edit golden snapshot files - always use `-regenerate-snapshots` flag. Snapshots capture exact output including invisible formatting (lipgloss padding, ANSI codes, trailing whitespace)
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
cmd/list/components_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 cmd/**/*_test.go : ALWAYS use `cmd.NewTestKit(t)` for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Applied to files:
cmd/list/components_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:
cmd/list/components_test.go
📚 Learning: 2024-12-08T14:26:16.972Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 828
File: pkg/schema/schema.go:98-100
Timestamp: 2024-12-08T14:26:16.972Z
Learning: The `ListConfig` columns array in the `Components` struct can be empty.
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/list/components_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:
cmd/list/components_test.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/list/components_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:
cmd/list/components_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:
internal/exec/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.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:
internal/exec/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.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/stack_processor_cache.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/stack_processor_cache.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/stack_processor_cache.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 : Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`
Applied to files:
internal/exec/stack_processor_cache.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:
internal/exec/stack_processor_cache.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
internal/exec/stack_processor_cache.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/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.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:
internal/exec/stack_processor_cache.go
🧬 Code graph analysis (1)
internal/exec/stack_processor_cache.go (4)
pkg/schema/schema.go (1)
BaseComponentConfig(845-860)pkg/merge/merge.go (1)
DeepCopyMap(29-45)pkg/perf/perf.go (1)
Track(121-138)errors/builder.go (1)
Build(24-37)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (15)
internal/exec/stack_processor_cache.go (2)
32-64: LGTM!Clean helper that covers all map fields from
BaseComponentConfig. Good consolidation of the deep copy logic.
156-227: LGTM!Clear functions properly synchronized. File content functions use the sentinel error pattern with good context. The
Range+Deleteapproach for clearingsync.Mapis the correct pattern for concurrent access scenarios.pkg/list/format/tree_instances.go (1)
1-222: All previous feedback addressed, code looks solid.The three issues from the previous review have all been resolved:
- Performance tracking is now in place (line 29)
- No duplicate constant declaration
- Comment formatting is correct
The implementation is clean with good separation of concerns. The tree rendering logic properly handles edge cases, produces deterministic output via sorting, and follows repository conventions for imports, comments, and function organization.
cmd/list/components_test.go (12)
3-25: initTestIO helper wiring looks solidThe new imports and
initTestIOhelper correctly initialize the I/O context, UI formatter, and data writer sorenderComponentspaths behave like real runs. No issues here.
232-325: Filter‑building tests cover key option combinations
TestBuildComponentFiltersdoes a good job exercising howComponentsOptionsmaps to filter counts, including the “abstract by default”,Typebeing authoritative, and the special handling ofType=allandAbstract=true. This should catch most wiring regressions between options and the filter set.
327-388: Column selection tests align with config semantics
TestGetComponentColumnsnicely covers default columns, CLI‑flag override, and config‑driven columns, and it explicitly handles the case whereListConfig.Columnsis empty, which matches prior schema learnings for this project. Based on learnings, this is exactly the behavior we want to lock in.
390-437: Sorter tests validate normal and error paths without string matching
TestBuildComponentSortersvalidates default sort, single/multi‑field specs, and the invalid‑spec error path while only checking for error presence (no string inspection), which matches the repo’s error‑testing guidelines.
439-505: Column flag parsing tests are comprehensive
TestParseColumnsFlagexercises empty, simple, named, auto‑wrapped, and mixed column specs, checking bothNameandValue. This gives good confidence that CLI--columnsinputs are normalized correctly before hitting the renderer.
507-578: parseColumnSpec tests cover core shapes and whitespace handling
TestParseColumnSpecthoroughly covers empty/whitespace specs, raw field names, named columns, auto‑wrapping, and more complex templates including functions. This is a solid behavioral contract for the spec parser.
580-661: Edge‑case coverage for parseColumnsFlag is well done
TestParseColumnsFlag_EdgeCasesusefully locks in behavior for empty strings, mixed empty/valid specs, underscores, digits, equals at the end, and templates witheqand conditionals. This should prevent subtle regressions in how we interpret unusual--columnsinputs.
663-722: Special‑character parsing tests set clear expectations
TestParseColumnSpec_SpecialCharactersdefines behavior for dotted, hyphenated, piped, conditional, ranged, and colon‑bearing specs. Even if some of these templates may be user‑responsibility at render time, it’s helpful that parsing semantics are explicitly tested here.
724-746: Verification of multiple parsed columns looks good
TestParseColumnsFlag_VerifyAllColumnsis a nice sanity check that mixed simple, named‑template, and named‑field specs all end up in the expected normalized form and order.
748-763: Column completion test is lightweight but appropriate
TestColumnsCompletionForComponentsvalidates thatcolumnsCompletionForComponentsreturns promptly with the expectedShellCompDirectiveNoFileCompeven when config cannot be loaded, without overreaching into full integration. That’s a good unit‑level check.
933-1017: Tri‑state enabled/locked filter tests exercise key combinations
TestRenderComponents_TriStateBoolFiltersdoes a good job invokingrenderComponentswith variousEnabled/Lockedtri‑state combinations and JSON format, ensuring the filter‑building and rendering paths handle all these cases without error. This complements the lower‑level filter tests nicely.
1019-1077: Type and abstract flag rendering behavior is well covered
TestRenderComponents_TypeFiltercoversTypefilters for specific types,all, and differentAbstractflag combinations against mixed real/abstract components. That’s a solid contract for how type/abstract options shape the rendered output set.
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)
cmd/list/components_test.go (1)
27-70: AddNewTestKit(t)to all test functions to isolate RootCmd state.Per the codebase-wide pattern, all cmd tests must use
_ = NewTestKit(t)at the beginning, including pure unit tests. This auto-cleans RootCmd state and prevents test pollution. Apply this toTestListComponentsFlags,TestListComponentsValidatesArgs, andTestListComponentsCommand.
🧹 Nitpick comments (1)
internal/exec/stack_processor_cache.go (1)
66-131: Base component config cache logic is solid; consider simplifying the slice return.The read/write paths both deep-copy maps and the inheritance slice, which keeps the cache immutable and race-free under the RWMutex. The extra pointer return for
ComponentInheritanceChain(*[]string) is a bit unusual in Go; unless there’s a strong caller requirement, returning the slice by value fromgetCachedBaseComponentConfigwould likely be simpler to work with.
📜 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 (2)
cmd/list/components_test.go(2 hunks)internal/exec/stack_processor_cache.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/list/components_test.go
**/*.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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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:
cmd/list/components_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
cmd.NewTestKit(t)for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Files:
cmd/list/components_test.go
🧠 Learnings (47)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-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:
cmd/list/components_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:
cmd/list/components_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:
cmd/list/components_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 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/list/components_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:
cmd/list/components_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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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:
cmd/list/components_test.go
📚 Learning: 2024-12-08T14:26:16.972Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 828
File: pkg/schema/schema.go:98-100
Timestamp: 2024-12-08T14:26:16.972Z
Learning: The `ListConfig` columns array in the `Components` struct can be empty.
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : NEVER use `assert.Contains(err.Error(), ...)` for error checking. ALWAYS use `assert.ErrorIs()` instead. NEVER use string comparison: `err.Error() == "..."` or `strings.Contains(err.Error(), ...)`
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.go
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*_test.go : Use `assert.ErrorIs(t, err, ErrSentinel)` for checking our own errors and stdlib errors. Only use string matching for third-party errors
Applied to files:
cmd/list/components_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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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:
cmd/list/components_test.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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/**/*_test.go : NEVER use pipe redirection (`2>&1`, `| head`, `| tail`) when running tests. Piping breaks TTY detection → ASCII fallback → wrong snapshots. NEVER manually edit golden snapshot files - always use `-regenerate-snapshots` flag. Snapshots capture exact output including invisible formatting (lipgloss padding, ANSI codes, trailing whitespace)
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
cmd/list/components_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 : All comments must end with periods (enforced by `godot` linter)
Applied to files:
cmd/list/components_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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 : NEVER delete existing comments without a very strong reason. Comments document why/how/what/where. Preserve helpful comments, update to match code changes, refactor for clarity, add context when modifying. Only acceptable removals: factually incorrect, code removed, duplicates obvious code, outdated TODO completed
Applied to files:
cmd/list/components_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 : Keep files small and focused (<600 lines). One cmd/impl per file. Co-locate tests. Never use `//revive:disable:file-length-limit`
Applied to files:
cmd/list/components_test.gointernal/exec/stack_processor_cache.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:
cmd/list/components_test.gointernal/exec/stack_processor_cache.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
cmd/list/components_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 cmd/**/*_test.go : ALWAYS use `cmd.NewTestKit(t)` for cmd tests. This auto-cleans RootCmd state (flags, args) and is required for any test touching RootCmd
Applied to files:
cmd/list/components_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:
cmd/list/components_test.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/list/components_test.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_cache.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_cache.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_cache.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:
internal/exec/stack_processor_cache.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_cache.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_cache.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:
internal/exec/stack_processor_cache.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_cache.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_cache.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:
internal/exec/stack_processor_cache.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
internal/exec/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.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_cache.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/stack_processor_cache.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/stack_processor_cache.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/stack_processor_cache.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
internal/exec/stack_processor_cache.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/stack_processor_cache.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:
internal/exec/stack_processor_cache.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_cache.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: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
internal/exec/stack_processor_cache.go
🧬 Code graph analysis (1)
cmd/list/components_test.go (7)
pkg/logger/log.go (2)
Helper(181-183)Fatalf(69-71)pkg/io/context.go (1)
NewContext(22-49)pkg/ui/formatter.go (1)
InitFormatter(47-69)pkg/data/data.go (1)
InitWriter(29-35)cmd/list/components.go (1)
ComponentsOptions(29-39)pkg/schema/schema.go (2)
ListConfig(971-976)ListColumnConfig(978-982)pkg/list/extract/components.go (1)
Components(18-47)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (11)
internal/exec/stack_processor_cache.go (5)
3-30: Imports and cache globals look consistent and thread-safe.Import grouping/aliases follow repo conventions, and the cache globals use RWMutex/sync.Map appropriately for concurrent access during a single command execution. No issues here.
32-64: Deep-copy helper correctly covers all mutable BaseComponentConfig maps.
deepCopyBaseComponentConfigMapsnow includes metadata, hooks, backend, and remote-state sections, so cached configs stay immutable even if callers mutate their copies later.
133-154: Compiled schema caching is straightforward and safe for reuse.The JSON schema cache uses an RWMutex with simple get/set helpers and treats the compiled schema as immutable shared state, which fits the intended per-command reuse pattern.
156-185: Cache clear helpers align well with test and command isolation needs.Each
Clear*Cachefunction takes the appropriate lock and resets the underlying structure, with perf tracking in place. This should play nicely with tests and repeated command runs.
187-227: File content caching and error handling follow project patterns nicely.
GetFileContentusessync.Mapcorrectly, handles both[]byteandstringdefensively, and wraps I/O failures witherrUtils.ErrReadFileplus path context. The uncached variant mirrors the same error pattern. This matches the sentinel-error and no-direct-IO guidelines well.cmd/list/components_test.go (6)
15-25: Helper implementation looks solid.The
initTestIOhelper correctly usest.Helper()and initializes the I/O context, UI formatter, and data writer needed for rendering tests. Usingt.Fatalf()for setup failures is appropriate in test helpers.
138-242: Excellent table-driven test coverage.
TestComponentsOptions_AllFieldsthoroughly exercises all option fields including tri-state boolean handling (nil, true, false) for Enabled and Locked. The careful pointer comparison logic (lines 224-234) correctly handles nil cases before dereferencing.
402-449: Error handling follows guidelines correctly.Test uses
assert.Error()for error presence checks rather than string-based assertions, which aligns with repository standards.
760-775: Completion test provides basic coverage.The test verifies that
columnsCompletionForComponentsreturns gracefully without a valid config, which is appropriate for a unit test. More comprehensive completion testing would occur in integration tests.
777-970: Comprehensive rendering test coverage.
TestRenderComponentsexercises the rendering pipeline across multiple formats (table, json, yaml, csv, tsv), includes error cases (invalid sort spec), and tests various filtering/sorting scenarios. The table-driven approach provides thorough coverage.
451-758: Exceptional coverage of column parsing.The suite of column parsing tests thoroughly exercises the new functionality including edge cases (empty strings, whitespace), special characters (dots, hyphens, pipes), and complex templates (conditionals, functions). This demonstrates strong attention to detail for the new column configuration feature.
|
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 |
|
These changes were released in v1.203.0-test.1. |
what
atmos.yamland--columnsCLI flag with Go template support--format=treeand--provenanceflags for import hierarchy displaywhy
--columns "Name={{ .component }}")references
Key Features
Configurable Columns
CLI Flags
--columns- Custom column definitions with Go templates--format- Output format (table, json, yaml, csv, tsv, tree)--sort- Sort specification (e.g.,Stack:asc,Component:desc)--provenance- Show import hierarchy in tree formatTest Coverage
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
UX
✏️ Tip: You can customize this high-level summary in your review settings.