Improve atmos list stacks #979
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/cmd_utils.go (1)
720-737: Simplify stack completion logic.The function has been improved but could be further simplified:
- Consider using early returns for error cases
- Add error logging for better debugging
func stackFlagCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { configAndStacksInfo := schema.ConfigAndStacksInfo{} atmosConfig, err := cfg.InitCliConfig(configAndStacksInfo, true) if err != nil { + u.LogDebug(fmt.Sprintf("Failed to initialize CLI config: %v", err)) return nil, cobra.ShellCompDirectiveNoFileComp } // Get all stacks stacksMap, err := e.ExecuteDescribeStacks(atmosConfig, "", nil, nil, nil, false, true, true, false, nil) if err != nil { + u.LogDebug(fmt.Sprintf("Failed to execute describe stacks: %v", err)) return nil, cobra.ShellCompDirectiveNoFileComp } // Filter stack names using configured base path stackNames := filterStackNames(stacksMap, atmosConfig.Stacks.BasePath) + if len(stackNames) == 0 { + u.LogDebug("No stack names found after filtering") + } return stackNames, cobra.ShellCompDirectiveNoFileComp }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cmd_utils.go(1 hunks)cmd/cmd_utils_test.go(1 hunks)pkg/list/list_stacks.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
cmd/cmd_utils_test.go (1)
1-59: Well-structured test cases with good coverage!The test implementation is thorough and covers essential scenarios:
- Filtering with standard base path
- Handling custom base paths
- Edge case with empty base path
pkg/list/list_stacks.go (4)
19-72: Well-documented metadata builder with comprehensive error handling.The function effectively organizes stack data into a consistent structure while handling nil values and nested components appropriately.
57-62: Add validation for stage extraction from stack name.The stage extraction logic should validate the stack name format before using it as a stage value.
86-111: Improve error handling in template parsing.The function silently returns false on template parsing errors, which could mask configuration issues.
296-317: Improve TSV handling using CSV writer.The TSV implementation could be simplified by reusing the CSV writer with a tab delimiter.
cmd/cmd_utils.go (1)
707-718: Avoid hardcoding path checks.The hardcoded check for base path prefix is not aligned with Atmos's configurable nature. This could break if users configure different stack paths.
| @@ -704,12 +704,36 @@ func showUsageExample(cmd *cobra.Command, details string) { | |||
| u.PrintInvalidUsageErrorAndExit(errors.New(details), suggestion) | |||
| } | |||
|
|
|||
| // filterStackNames filters out file paths and returns valid stack names | |||
| func filterStackNames(stacksMap map[string]any, basePath string) []string { | |||
There was a problem hiding this comment.
Let’s revise this function name. We want to add proper filtering later (not this PR), but this is filtering on paths. Also, it’s not clear to me why we need this. Can you add a comment explaining why we need this?
There was a problem hiding this comment.
I guess this wold get replaced once we refactor the full stacks path on the discussion bellow
| stackNames := make([]string, 0) | ||
| for stackName := range stacksMap { | ||
| // Skip if it's a file path (starts with base path) | ||
| if basePath != "" && strings.HasPrefix(stackName, basePath+"/") { |
There was a problem hiding this comment.
No, this is not right either. Stacks can exist anywhere, what we include and exclude is entirely config driven. We are not consulting that configuration, and deriving an implementation that doesn’t correspond to the documentation. We should have a centralized function for this and it relates to stacks not general utils, IMO.
There was a problem hiding this comment.
There was a problem hiding this comment.
so I think on this case would be best having that function inside stacks pkg and then reuse FindAllStackConfigsInPaths from config to find the paths
There was a problem hiding this comment.
@aknysh please let me know your opinion of whats the best approach to refactor this, I see that we also have getConfigAndStacksInfo in utils
|
This will be on hold until we merge ##1036 which includes the pkgs needed for refactoring |
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
|
will reopen another PR |
what
why
Evidence:
references
Summary by CodeRabbit
New Features
listsection in the configuration files.Bug Fixes
Documentation
Tests