Implement: atmos list values #1036
Conversation
📝 WalkthroughWalkthroughThis update introduces two new CLI commands— Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CLI (listValuesCmd/listVarsCmd)
participant A as Atmos Config Checker
participant F as FilterAndListValues
participant O as Output Formatter
U->>C: Invoke command with flags
C->>A: Check Atmos configuration
A-->>C: Configuration validated
C->>F: Call FilterAndListValues with parameters
F-->>C: Return filtered values or error
C->>O: Format and display output
O-->>U: Present result with success/warning colors
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (11)
pkg/list/list_workflows.go (1)
202-230: Simplify the default case logic.The default case handles both empty format and "table" format with duplicated string joining logic. This can be simplified to reduce code duplication.
Consider this refactor:
default: + var output strings.Builder + output.WriteString(strings.Join(header, delimiter) + utils.GetLineEnding()) + for _, row := range rows { + output.WriteString(strings.Join(row, delimiter) + utils.GetLineEnding()) + } + // If format is empty or "table", use table format if format == "" && term.IsTTYSupportForStdout() { // Create a styled table for TTY t := table.New(). // ... existing table configuration ... Headers(header...). Rows(rows...) return t.String() + utils.GetLineEnding(), nil } - // Default to simple tabular format for non-TTY or when format is explicitly "table" - var output strings.Builder - output.WriteString(strings.Join(header, delimiter) + utils.GetLineEnding()) - for _, row := range rows { - output.WriteString(strings.Join(row, delimiter) + utils.GetLineEnding()) - } return output.String(), nilpkg/list/list_values.go (3)
23-31: Consider removing the unused function.The static analysis tool confirms that
getMapKeysis unused. If there's no future usage planned, removing it will reduce clutter.-// getMapKeys returns a sorted slice of map keys -func getMapKeys(m map[string]interface{}) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - sort.Strings(keys) - return keys -}🧰 Tools
🪛 golangci-lint (1.62.2)
24-24: func
getMapKeysis unused(unused)
34-324: Refactor the large function.
FilterAndListValueshandles multiple steps: filtering stacks, applying queries, formatting output, etc. Splitting it into smaller helper functions would improve readability and maintainability.
265-272: Handle CSV field quoting.Concatenating fields with a plain delimiter can corrupt CSV data if the field itself contains the delimiter or newline. Consider quoting fields for robust CSV output.
pkg/list/list_values_test.go (1)
56-148: Expand test coverage.You might test nested or array-based query paths to confirm filtering works for complex data structures. It would further validate the code’s resilience.
cmd/list_values.go (2)
35-63: Avoid repeated flag error-handling.Error-handling for each flag is repeated. Consolidating this into a small helper would reduce duplication and simplify reading.
95-116: Evaluate independent command structure.
listVarsCmdsets a single flag before reusinglistValuesCmd.Run. This works but might complicate future expansions. Consider a dedicated or unified approach for better clarity.website/docs/cli/commands/list/list-values.mdx (4)
31-42: Consider enhancing flag documentation with example values.While the flag descriptions are clear, adding example values would make them more user-friendly.
<dt><code>--query string</code></dt> - <dd>JMESPath query to filter values (e.g., ".vars" to show only variables)</dd> + <dd>JMESPath query to filter values (e.g., ".vars" to show only variables, ".config.vpc" to show VPC configuration)</dd> <dt><code>--format string</code></dt> - <dd>Output format: `table`, `json`, `csv`, `tsv` (default "`table`")</dd> + <dd>Output format: `table`, `json`, `csv`, `tsv` (default "`table`"). Example: --format=json</dd>
58-58: Replace TODO placeholder with actual JMESPath query examples.The placeholder needs to be replaced with practical JMESPath query examples to help users understand how to filter values effectively.
Would you like me to help generate some practical JMESPath query examples? Here's a suggestion:
-TODO: define more outputs +# Show only networking configuration +atmos list values vpc --query ".config.networking" + +# Filter for specific environment variables +atmos list values vpc --query ".vars | [?contains(name, 'ENV')]" + +# Show components with specific tags +atmos list values vpc --query ".metadata.tags"
86-86: Add example command output to enhance documentation.Replace the TODO with actual command output to help users understand what to expect.
Would you like me to help generate an example output? Here's a suggestion:
-TODO: define example output +Component: vpc + +┌─────────────┬──────────────┬──────────────┬──────────────┐ +│ Key │ dev-ue1 │ staging-ue1 │ prod-ue1 │ +├─────────────┼──────────────┼──────────────┼──────────────┤ +│ cidr_block │ 10.0.0.0/16 │ 10.1.0.0/16 │ 10.2.0.0/16 │ +│ enable_flow │ true │ true │ true │ +│ max_azs │ 3 │ 3 │ 3 │ +└─────────────┴──────────────┴──────────────┴──────────────┘
91-92: Enhance typography in related commands section.Replace hyphens with em dashes for better readability.
-[atmos list components](/cli/commands/list/component) - List available components -[atmos describe component](/cli/commands/describe/component) - Show detailed information about a component +[atmos list components](/cli/commands/list/component) — List available components +[atmos describe component](/cli/commands/describe/component) — Show detailed information about a component🧰 Tools
🪛 LanguageTool
[typographical] ~91-~91: To join two clauses or introduce examples, consider using an em dash.
Context: ...omponents](/cli/commands/list/component) - List available components - [atmos descr...(DASH_RULE)
[typographical] ~92-~92: To join two clauses or introduce examples, consider using an em dash.
Context: ...onent](/cli/commands/describe/component) - Show detailed information about a compon...(DASH_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/list_values.go(1 hunks)pkg/list/list_values.go(1 hunks)pkg/list/list_values_test.go(1 hunks)pkg/list/list_workflows.go(1 hunks)pkg/list/list_workflows_test.go(7 hunks)website/docs/cli/commands/list/list-values.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/commands/list/list-values.mdx
[typographical] ~91-~91: To join two clauses or introduce examples, consider using an em dash.
Context: ...omponents](/cli/commands/list/component) - List available components - [atmos descr...
(DASH_RULE)
[typographical] ~92-~92: To join two clauses or introduce examples, consider using an em dash.
Context: ...onent](/cli/commands/describe/component) - Show detailed information about a compon...
(DASH_RULE)
🪛 golangci-lint (1.62.2)
pkg/list/list_values.go
24-24: func getMapKeys is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/list/list_workflows.go (1)
25-25: LGTM! New format constants follow the established pattern.The addition of
FormatYAMLandFormatTSVconstants maintains consistency with the existing format naming convention.Also applies to: 27-27
pkg/list/list_workflows_test.go (1)
76-76: LGTM! File permission notation updated to modern format.The change to use the
0oprefix for octal file permissions is more explicit and follows modern Go conventions.Also applies to: 89-89, 100-100, 118-118, 136-136, 310-310
website/docs/cli/commands/list/list-values.mdx (1)
1-28: Strong work on the command description!The command description effectively communicates the purpose and value proposition of the
atmos list valuescommand. The tabular view explanation is particularly clear and helpful.
| ) | ||
|
|
||
| // ValidateFormat checks if the given format is supported | ||
| func ValidateFormat(format string) error { | ||
| if format == "" { | ||
| return nil | ||
| } | ||
| validFormats := []string{FormatTable, FormatJSON, FormatCSV} | ||
| validFormats := []string{FormatTable, FormatJSON, FormatYAML, FormatCSV, FormatTSV} |
There was a problem hiding this comment.
Add handling for new formats in FilterAndListWorkflows.
While the formats are validated here, the FilterAndListWorkflows function's switch statement (line 171) doesn't handle the new "yaml" and "tsv" formats, causing them to fall through to the default case.
Add cases for the new formats:
switch format {
case "json":
// ... existing json handling ...
+case "yaml":
+ // Convert to YAML format
+ type workflow struct {
+ File string `yaml:"file"`
+ Name string `yaml:"name"`
+ Description string `yaml:"description"`
+ }
+ var workflows []workflow
+ for _, row := range rows {
+ workflows = append(workflows, workflow{
+ File: row[0],
+ Name: row[1],
+ Description: row[2],
+ })
+ }
+ yamlBytes, err := yaml.Marshal(workflows)
+ if err != nil {
+ return "", fmt.Errorf("error formatting YAML output: %w", err)
+ }
+ return string(yamlBytes), nil
+
+case "tsv":
+ // Use tab as delimiter for TSV output
+ var output strings.Builder
+ output.WriteString(strings.Join(header, "\t") + utils.GetLineEnding())
+ for _, row := range rows {
+ output.WriteString(strings.Join(row, "\t") + utils.GetLineEnding())
+ }
+ return output.String(), nil
+
case "csv":
// ... existing csv handling ...Also applies to: 41-41
| { | ||
| name: "valid yaml format", | ||
| format: "yaml", | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
Add test case for TSV format.
While the test case for YAML format is added, a test case for the new TSV format is missing.
Add the missing test case:
{
name: "valid yaml format",
format: "yaml",
wantErr: false,
},
+ {
+ name: "valid tsv format",
+ format: "tsv",
+ wantErr: false,
+ },
{
name: "invalid format",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "valid yaml format", | |
| format: "yaml", | |
| wantErr: false, | |
| }, | |
| { | |
| name: "valid yaml format", | |
| format: "yaml", | |
| wantErr: false, | |
| }, | |
| { | |
| name: "valid tsv format", | |
| format: "tsv", | |
| wantErr: false, | |
| }, | |
| { | |
| name: "invalid format", |
| Use: "vars [component]", | ||
| Short: "List component vars across stacks (alias for 'list values --query .vars')", | ||
| Long: "List vars for a component across all stacks where it is used", | ||
| Example: "atmos list vars vpc\n" + |
There was a problem hiding this comment.
Please use the new style of embedding the examples. See other commands for how to do that.
See: #959
|
|
||
| ```shell | ||
| > atmos list vars vpc | ||
| TODO: define example output |
|
|
||
| List values with a custom JMESPath query: | ||
| ```shell | ||
| TODO: define more outputs |
| ``` | ||
|
|
||
| ```shell | ||
| atmos list values vpc --query .config |
There was a problem hiding this comment.
We don't have .config but we have .settings
| atmos list values vpc --query .config | |
| atmos list values vpc --query .settings |
Please ensure it works with the advanced QuickStart
what
why
Evidence:
references
Summary by CodeRabbit
New Features
Documentation
Tests