Improve atmos list stacks #979
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhanced configuration and output capabilities for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Learnings (1)internal/exec/describe_stacks.go (1)⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (12)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/list/list_stacks.go (1)
202-236: Enhance table rendering and consider performance.
The TTY support is a nice touch. Also, in lines 226-228, you can apply the suggestedcopy(headerRow, headers)improvement to simplify the loop.-for i, h := range headers { - headerRow[i] = h -} +copy(headerRow, headers)🧰 Tools
🪛 golangci-lint (1.62.2)
226-226: S1001: should use copy(to, from) instead of a loop
(gosimple)
cmd/list_stacks.go (1)
33-34: Consider adding error handling for the flag parsing.While the code works, it silently ignores any flag parsing errors. Consider handling these errors to provide better user feedback.
- formatFlag, _ := cmd.Flags().GetString("format") - delimiterFlag, _ := cmd.Flags().GetString("delimiter") + formatFlag, err := cmd.Flags().GetString("format") + if err != nil { + u.PrintMessageInColor(fmt.Sprintf("Error getting format flag: %v", err), theme.Colors.Error) + return + } + delimiterFlag, err := cmd.Flags().GetString("delimiter") + if err != nil { + u.PrintMessageInColor(fmt.Sprintf("Error getting delimiter flag: %v", err), theme.Colors.Error) + return + }Also applies to: 49-49
pkg/list/list_components_test.go (1)
48-55: Consider adding more test cases.While the current test case is good, consider adding tests for:
- Different format options (json, csv)
- Custom delimiters
- Invalid template expressions
atmos.yaml (1)
79-88: LGTM! Well-structured column configuration.The list configuration effectively uses Go templates to format the output. The column names and values are clear and meaningful.
Consider adding documentation for template variables.
It would be helpful to add comments explaining the available template variables and their usage.
list: + # Available template variables: + # - .atmos_stack: The full stack name + # - .vars: Map of stack variables (e.g., tenant, environment) + # - .atmos_stack_file: The stack configuration file path columns:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
atmos.yaml(1 hunks)cmd/list_stacks.go(3 hunks)pkg/list/list_components_test.go(1 hunks)pkg/list/list_stacks.go(2 hunks)pkg/list/list_stacks_test.go(2 hunks)pkg/schema/schema.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(2 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/list/list_stacks.go
226-226: S1001: should use copy(to, from) instead of a loop
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (16)
pkg/list/list_stacks.go (8)
4-4: Imports look good.
No problems found.Also applies to: 8-8, 10-10
18-30: Validate user inputs and fallback logic.
This section elegantly checksformatand falls back tolistConfig.Format. Good clarity and error handling.
49-82: Consolidation of stack information is thorough.
Mergingvarsand component-specific vars ensures comprehensive data instackInfo. This is well-structured.
85-127: All-stacks listing logic is consistent.
Inclusion of environment details and dynamic fallback to.yamlfiles aligns with current approach.
129-140: Graceful handling of empty stack lists.
The conditional check returns descriptive messages. Looks good.
141-150: Default columns are adequately defined.
The fallback scenario is well-handled.
151-174: Template parsing and execution.
Error messaging for template issues is concise and clear. Good job.
177-201: JSON and CSV generation is well-structured.
Appending rows is intuitive, and error checks are clean.tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (2)
28-30: New list configuration section.
This neatly decouples listing requirements understacks. Good design.
38-44: Path changes applied.
Strong consistency with the new user environment. No issues spotted.pkg/list/list_stacks_test.go (2)
28-37: Validating empty format and columns in TestListStacks.
The coverage forFilterAndListStackswith a basiclistConfigis appropriate.
54-63: Testing component-based listing with columns.
Looked for potential missing negative tests, but the coverage is still acceptable.cmd/list_stacks.go (2)
22-25: LGTM! Clear and helpful examples.The examples effectively demonstrate the various output format options available to users.
61-62: LGTM! Well-documented flags.The flags are properly registered with clear descriptions and appropriate default values.
pkg/schema/schema.go (1)
152-157: LGTM! Well-structured schema definition.The
Stacksstruct is properly organized with clear field tags and documentation.pkg/list/list_components_test.go (1)
57-57: LGTM! Proper test assertion chain.The test properly validates the output and error handling.
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/list/list_stacks_test.go (2)
28-35: Consider extracting duplicated test configuration.The list configuration is identical in both test functions. Let's refactor this to improve maintainability.
+// Common test configuration +var defaultTestConfig = schema.ListConfig{ + Format: "", + Columns: []schema.ListColumnConfig{ + {Name: "Stack", Value: "{{ .atmos_stack }}"}, + {Name: "File", Value: "{{ .atmos_stack_file }}"}, + }, +} func TestListStacks(t *testing.T) { // ... - listConfig := schema.ListConfig{ - Format: "", - Columns: []schema.ListColumnConfig{ - {Name: "Stack", Value: "{{ .atmos_stack }}"}, - {Name: "File", Value: "{{ .atmos_stack_file }}"}, - }, - } + listConfig := defaultTestConfig // ... } func TestListStacksWithComponent(t *testing.T) { // ... - listConfig := schema.ListConfig{ - Format: "", - Columns: []schema.ListColumnConfig{ - {Name: "Stack", Value: "{{ .atmos_stack }}"}, - {Name: "File", Value: "{{ .atmos_stack_file }}"}, - }, - } + listConfig := defaultTestConfig // ... }Also applies to: 54-61
111-113: Strengthen error handling tests.The current error handling is basic. Consider adding specific error case assertions.
t.Run(test.name, func(t *testing.T) { result, err := FilterAndListStacks(context, stacksBasePath, test.config, stackType, component) - assert.NoError(t, err) - assert.Equal(t, test.expected, result) + if test.expectedError != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), test.expectedError) + } else { + assert.NoError(t, err) + assert.Equal(t, test.expected, result) + } })website/docs/cli/commands/list/list-stacks.mdx (2)
52-60: Enhance documentation with more configuration examples.The current example is good but limited. Consider adding examples for:
- Multiple columns configuration
- Different output formats
- Complex template expressions
```yaml stacks: display: columns: - name: Stack value: '{{ .atmos_stack }}' - name: Tenant value: '{{ index .vars "tenant" }}' + - name: Environment + value: '{{ index .vars "environment" | default "N/A" }}' + - name: Components + value: '{{ range $key, $value := .components }}{{ $key }}, {{ end }}' + format: "table" # or "json", "csv"--- `89-95`: **Expand example outputs section.** Consider adding examples for different output formats and scenarios. ```diff $ atmos list stacks STACK TENANT dev acme staging acme prod acme-corp + +# JSON output example +$ atmos list stacks --format json +[ + { + "Stack": "dev", + "Tenant": "acme", + "Environment": "development" + } + ... +] + +# CSV output example +$ atmos list stacks --format csv +Stack,Tenant,Environment +dev,acme,development +...pkg/list/list_stacks.go (2)
20-29: Strengthen format validation.The format validation could be more robust by adding constants for supported formats.
+const ( + FormatTable = "table" + FormatJSON = "json" + FormatCSV = "csv" +) +var supportedFormats = map[string]bool{ + FormatTable: true, + FormatJSON: true, + FormatCSV: true, + "": true, // empty string defaults to table +} func ValidateFormat(format string) error { - // existing validation + if !supportedFormats[format] { + return fmt.Errorf("unsupported format %q, supported formats: table, json, csv", format) + } return nil }
223-227: Optimize header copying.The current implementation uses a loop to copy headers. This can be simplified using the built-in copy function.
-headerRow := make([]string, len(headers)) -for i, h := range headers { - headerRow[i] = h -} +headerRow := make([]string, len(headers)) +copy(headerRow, headers)🧰 Tools
🪛 golangci-lint (1.62.2)
224-224: S1001: should use copy(to, from) instead of a loop
(gosimple)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/list/list_stacks.go(2 hunks)pkg/list/list_stacks_test.go(3 hunks)website/docs/cli/commands/list/list-stacks.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/commands/list/list-stacks.mdx
[uncategorized] ~68-~68: Possible missing comma found.
Context: ...tion provides a safer way to access map values as it won't cause a template error if t...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 golangci-lint (1.62.2)
pkg/list/list_stacks.go
224-224: S1001: should use copy(to, from) instead of a loop
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
website/docs/cli/commands/list/list-stacks.mdx (4)
50-60: Add documentation for supported output formats.The configuration example is clear, but we should also document the supported output formats (JSON, CSV, table) mentioned in the implementation.
Add a section about output formats like this:
The command output can be customized using template variables in your `atmos.yaml` configuration. Here's an example of how to configure the display format to add a custom field called "Tenant": +You can specify the output format using the `--output` flag: +```shell +atmos list stacks --output json # JSON format +atmos list stacks --output csv # CSV format +atmos list stacks --output table # Table format (default) +``` + ```yaml
56-58: Document the.atmos_stacktemplate variable.The example uses
.atmos_stackbut doesn't explain what this variable represents.Add a brief explanation of the
.atmos_stackvariable and other available template variables.
73-77: Clarify configuration precedence.The documentation should explain how custom configurations interact with default settings. Do custom columns replace or append to the defaults?
Add a note about configuration behavior:
You can customize the displayed columns using the `list.stacks.columns` configuration in `atmos.yaml`. + +:::note Configuration Behavior +Custom column configurations will completely replace the default columns. If you want to keep the default columns while adding new ones, you need to include them in your configuration. +:::
68-68: Add comma for improved readability.Add a comma after "map values" to improve sentence flow.
-The `index` function provides a safer way to access map values as it won't cause a template error if the key doesn't exist. +The `index` function provides a safer way to access map values, as it won't cause a template error if the key doesn't exist.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Possible missing comma found.
Context: ...tion provides a safer way to access map values as it won't cause a template error if t...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/docs/cli/commands/list/list-stacks.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/commands/list/list-stacks.mdx
[uncategorized] ~68-~68: Possible missing comma found.
Context: ...tion provides a safer way to access map values as it won't cause a template error if t...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
| - name: Stack | ||
| value: '{{ .atmos_stack }}' | ||
| - name: Tenant | ||
| value: '{{ index .vars "tenant" }}' |
There was a problem hiding this comment.
Can you confirm other sections like settings are accessible?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/list/list_stacks.go (2)
31-80: Simplify component filtering logic.While the helper function nicely addresses the duplication issue, the component filtering logic could be more concise.
Consider this more streamlined approach:
- componentFound := false - for _, componentSection := range components { - if compSection, ok := componentSection.(map[string]any); ok { - if _, exists := compSection[component]; exists { - componentFound = true - break - } - } - } - if componentFound { + if utils.ComponentExistsInComponents(components, component) { stackInfo := createStackInfo(stackName, v2) filteredStacks = append(filteredStacks, stackInfo) - }This assumes adding a helper function:
// utils/component.go func ComponentExistsInComponents(components map[string]any, component string) bool { for _, section := range components { if compSection, ok := section.(map[string]any); ok { if _, exists := compSection[component]; exists { return true } } } return false }
137-193: Extract styling constants for better maintainability.The TTY-aware table styling is great, but the padding values could be extracted into constants.
Consider this refactor:
+const ( + tableCellPaddingLeft = 1 + tableCellPaddingRight = 1 +) StyleFunc(func(row, col int) lipgloss.Style { - style := lipgloss.NewStyle().PaddingLeft(1).PaddingRight(1) + style := lipgloss.NewStyle(). + PaddingLeft(tableCellPaddingLeft). + PaddingRight(tableCellPaddingRight)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/list/list_stacks.go(1 hunks)pkg/list/list_stacks_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_stacks_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/list/list_stacks.go (3)
18-30: LGTM! Clean function signature and validation.Early format validation and graceful fallback to config-specified format shows good defensive programming.
81-100: LGTM! Good sorting and default column handling.The implementation provides sensible defaults while maintaining flexibility for custom configurations.
101-136: LGTM! Excellent template optimization.Pre-parsing templates outside the loop is a great performance optimization. The error handling is thorough and informative.
This addresses the template optimization suggestion from the previous review.
|
|
@osterman latest updates view
|
@osterman please note also that now I have decided to not display the values on table and json if they don't exist instead of displaying "" this will make the output cleaner, also for json output I did not found any way to remove this green color not totally sure where this coming out as I already removed all the styles |
|
@Cerebrovinny any additional tests we should add since this was not caught? |
|
what
why
Evidence:
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
--formatto specify output format (table, JSON, CSV).--delimiterto set column separator.listsection understacksandworkflows.Improvements