Implement: atmos list values #1036
Conversation
📝 WalkthroughWalkthroughThis update introduces a series of new CLI commands and supporting modules for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CmdHandler
participant ListLogic
participant Formatter
participant Output
User->>CLI: Execute "atmos list values <component>"
CLI->>CmdHandler: Parse arguments and flags
CmdHandler->>ListLogic: Call FilterAndListValues with options
ListLogic-->>CmdHandler: Return values/errors
CmdHandler->>Formatter: Format data based on selected output (table/json/yaml/csv/tsv)
Formatter-->>CmdHandler: Return formatted string
CmdHandler->>Output: Print result
Assessment against linked issues
Possibly related issues
Possibly related PRs
✨ Finishing Touches
🪧 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.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
cmd/list_values_internal_test.go (1)
66-119: Consider reducing duplication in test code.The
TestListVarsFlagsfunction contains significant duplication withTestListValuesFlags. Consider extracting the common flag verification logic into a helper function to improve maintainability.func verifyFlags(t *testing.T, cmd *cobra.Command) { formatFlag := cmd.PersistentFlags().Lookup("format") assert.NotNil(t, formatFlag, "Expected format flag to exist") assert.Equal(t, "", formatFlag.DefValue) // Similar verification for other flags... }Then call this function from both test functions.
cmd/list_metadata.go (1)
53-127: Consider refactoring to reduce cyclomatic complexity.The
listMetadatafunction has a cyclomatic complexity of 13 (above the max threshold of 10). Consider extracting flag processing into a separate function to improve readability and reduce complexity.func getProcessingFlags(cmd *cobra.Command) (bool, bool) { processTemplates := true processYamlFunctions := true var err error if cmd.Flags().Lookup("process-templates") != nil { processTemplates, err = cmd.Flags().GetBool("process-templates") if err != nil { log.Warn("failed to get process-templates flag, using default true", "error", err) } } if cmd.Flags().Lookup("process-functions") != nil { processYamlFunctions, err = cmd.Flags().GetBool("process-functions") if err != nil { log.Warn("failed to get process-functions flag, using default true", "error", err) } } return processTemplates, processYamlFunctions }This would make the main function more concise and easier to maintain.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 53-53:
cyclomatic: function listMetadata has cyclomatic complexity 13 (> max enabled 10)cmd/markdown/atmos_list_vars_usage.md (1)
1-2: Consider using an em dash or consistent punctuation.The current lines use the en dash “–” to introduce bullet points or lists (e.g.,
– List all variables for a component). For stylistic consistency, you might replace them with an em dash “—” or a simple “-” to match typical Markdown conventions.Also applies to: 6-7, 11-12, 16-17, 24-25, 29-30, 34-35, 39-40
🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all variables for a component ``` ...(DASH_RULE)
cmd/list_settings.go (3)
53-121: Refactor to reduce cyclomatic complexity.The
listSettingsfunction has a cyclomatic complexity of 12, which exceeds the maximum enabled complexity of 10. Consider breaking it down into smaller helper functions to improve maintainability.func listSettings(cmd *cobra.Command) (string, error) { + // Extract flags from command + flags, err := extractListSettingsFlags(cmd) + if err != nil { + return "", err + } + + // Initialize CLI config and retrieve stacks + stacksMap, err := getStacksForSettings(flags.processTemplates, flags.processYamlFunctions) + if err != nil { + return "", err + } + + // Log the settings query + logSettingsQuery(flags, stacksMap) + + // Process and filter settings + return filterAndReturnSettings(stacksMap, flags) + } + + type listSettingsFlags struct { + commonFlags fl.CommonListFlags + processTemplates bool + processYamlFunctions bool + } + + func extractListSettingsFlags(cmd *cobra.Command) (*listSettingsFlags, error) { // Get common flags commonFlags, err := fl.GetCommonListFlags(cmd) if err != nil { return "", &errors.CommonFlagsError{Cause: err} } // Get template and function processing flags processTemplates := true if cmd.Flags().Lookup("process-templates") != nil { processTemplates, err = cmd.Flags().GetBool("process-templates") if err != nil { log.Warn("failed to get process-templates flag, using default true", "error", err) } } processYamlFunctions := true if cmd.Flags().Lookup("process-functions") != nil { processYamlFunctions, err = cmd.Flags().GetBool("process-functions") if err != nil { log.Warn("failed to get process-functions flag, using default true", "error", err) } } if f.Format(commonFlags.Format) == f.FormatCSV && commonFlags.Delimiter == f.DefaultTSVDelimiter { commonFlags.Delimiter = f.DefaultCSVDelimiter } + + return &listSettingsFlags{ + commonFlags: commonFlags, + processTemplates: processTemplates, + processYamlFunctions: processYamlFunctions, + }, nil + } + + func getStacksForSettings(processTemplates, processYamlFunctions bool) (map[string]interface{}, error) { // Initialize CLI config configAndStacksInfo := schema.ConfigAndStacksInfo{} atmosConfig, err := config.InitCliConfig(configAndStacksInfo, true) if err != nil { return "", &errors.InitConfigError{Cause: err} } // Get all stacks stacksMap, err := e.ExecuteDescribeStacks(atmosConfig, "", nil, nil, nil, false, processTemplates, processYamlFunctions, false, nil) if err != nil { return "", &errors.DescribeStacksError{Cause: err} } + return stacksMap, nil + } + + func logSettingsQuery(flags *listSettingsFlags, stacksMap map[string]interface{}) { // Log the settings query log.Info("Filtering settings", "query", commonFlags.Query, "maxColumns", commonFlags.MaxColumns, "format", commonFlags.Format, "stackPattern", commonFlags.Stack, "processTemplates", processTemplates, "processYamlFunctions", processYamlFunctions) + } + + func filterAndReturnSettings(stacksMap map[string]interface{}, flags *listSettingsFlags) (string, error) { // Use empty query to avoid further processing since handleComponentProperties will extract the settings output, err := l.FilterAndListValues(stacksMap, &l.FilterOptions{ Component: "settings", Query: commonFlags.Query, IncludeAbstract: false, MaxColumns: commonFlags.MaxColumns, FormatStr: commonFlags.Format, Delimiter: commonFlags.Delimiter, StackPattern: commonFlags.Stack, }) if err != nil { if u.IsNoValuesFoundError(err) { return "", &errors.NoSettingsFoundError{Query: commonFlags.Query} } return "", &errors.SettingsFilteringError{Cause: err} } return output, nil🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 53-53:
cyclomatic: function listSettings has cyclomatic complexity 12 (> max enabled 10)
103-112: Set default query for improved usability.The code lacks a default query, which could be useful when users don't specify one. Consider setting a default query of
.settingsto match the command's purpose.// Use empty query to avoid further processing since handleComponentProperties will extract the settings + query := commonFlags.Query + if query == "" { + query = ".settings" + } + output, err := l.FilterAndListValues(stacksMap, &l.FilterOptions{ Component: "settings", - Query: commonFlags.Query, + Query: query, IncludeAbstract: false, MaxColumns: commonFlags.MaxColumns, FormatStr: commonFlags.Format, Delimiter: commonFlags.Delimiter, StackPattern: commonFlags.Stack, })
42-46: Consider using a constant for default column limit.The
--max-columnsflag uses a hardcoded value of 10. Consider using a named constant for better maintainability.+// DefaultMaxColumns is the default number of columns to display +const DefaultMaxColumns = 10 + func init() { fl.AddCommonListFlags(listSettingsCmd) // Add template and function processing flags - listSettingsCmd.PersistentFlags().Bool("process-templates", true, "Enable/disable Go template processing in Atmos stack manifests when executing the command") + listSettingsCmd.PersistentFlags().Bool("process-templates", true, "Enable/disable Go template processing in Atmos stack manifests when executing the command")cmd/markdown/atmos_list_settings_usage.md (1)
1-1:❓ Verification inconclusive
Consider using em dashes for consistency.
The static analysis tool suggests using em dashes (—) instead of en dashes (–) in dialogues and enumerations. If this follows project conventions, consider updating all dash instances in this file.
-– List all settings +— List all settings
🏁 Script executed:
#!/bin/bash # Check if em dashes are used in other markdown files rg --type md "^—" --countLength of output: 27
Below is an updated shell script that checks for both en dashes and em dashes in markdown files—this should give us a clearer picture of what the rest of the codebase is using. Please run it and verify its output manually:
Once you’ve verified the output, please confirm whether the project convention is indeed to use em dashes for dialogues and enumerations. If so, the suggested replacement in
cmd/markdown/atmos_list_settings_usage.mdremains valid:-– List all settings +— List all settingsLooking forward to your manual verification results.
🏁 Script executed:
#!/bin/bash echo "Counting lines starting with em dashes (—) in markdown files:" rg --type md "^—" --count echo "Counting lines starting with en dashes (–) in markdown files:" rg --type md "^–" --countLength of output: 662
Confirming Dash Style Consistency Across Markdown Files
Our static analysis tool recommends using em dashes (—) for dialogues and enumerations. However, a repository-wide check shows that several markdown files—such as in
cmd/markdown/atmos_terraform_usage.md,cmd/markdown/atmos_workflow_usage.md, and others—consistently start with en dashes (–). This suggests that the current project convention favors en dashes.Before updating
cmd/markdown/atmos_list_settings_usage.md, please confirm whether you intend to switch to em dashes project-wide. If so, we should apply the change consistently across all affected markdown files. Otherwise, it might be best to retain the en dash to maintain uniformity.-– List all settings +— List all settingsPlease verify the intended dash convention and adjust accordingly.
🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all settings ``` $ atmos list set...(DASH_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/list_metadata.go(1 hunks)cmd/list_settings.go(1 hunks)cmd/list_values.go(1 hunks)cmd/list_values_internal_test.go(1 hunks)cmd/markdown/atmos_list_metadata_usage.md(1 hunks)cmd/markdown/atmos_list_settings_usage.md(1 hunks)cmd/markdown/atmos_list_values_usage.md(1 hunks)cmd/markdown/atmos_list_vars_usage.md(1 hunks)pkg/list/list_values.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_values.go
🧰 Additional context used
🧠 Learnings (1)
cmd/list_settings.go (2)
Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2025-03-18T00:20:29.426Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
Learnt from: osterman
PR: cloudposse/atmos#1036
File: cmd/list_settings.go:46-50
Timestamp: 2025-03-18T00:20:29.426Z
Learning: In the Atmos project, -1 is used as a special value to denote unlimited columns in table outputs, following common software conventions.
🧬 Code Definitions (3)
cmd/list_values.go (2)
pkg/list/flags/flags.go (1) (1)
AddCommonListFlags(18:24)pkg/list/list_values.go (2) (2)
FilterOptions(47:55)FilterAndListValues(58:88)
cmd/list_metadata.go (4)
pkg/list/errors/types.go (19) (19)
QueryError(32:35)e(14:19)e(27:29)e(37:39)e(41:43)e(51:53)e(55:57)e(64:66)e(73:75)e(77:79)e(86:88)e(90:92)e(99:101)e(103:105)e(112:114)e(116:118)e(125:127)NoMetadataFoundError(60:62)MetadataFilteringError(69:71)pkg/list/format/formatter.go (2) (2)
Format(8:8)FormatCSV(14:14)pkg/list/list_values.go (3) (3)
FilterAndListValues(58:88)FilterOptions(47:55)IsNoValuesFoundError(342:345)pkg/list/utils/utils.go (1) (1)
IsNoValuesFoundError(8:11)
cmd/list_settings.go (4)
pkg/list/flags/flags.go (1) (1)
AddCommonListFlags(18:24)pkg/list/errors/types.go (19) (19)
CommonFlagsError(82:84)InitConfigError(95:97)e(14:19)e(27:29)e(37:39)e(41:43)e(51:53)e(55:57)e(64:66)e(73:75)e(77:79)e(86:88)e(90:92)e(99:101)e(103:105)e(112:114)e(116:118)e(125:127)SettingsFilteringError(130:132)pkg/list/list_values.go (3) (3)
FilterAndListValues(58:88)FilterOptions(47:55)IsNoValuesFoundError(342:345)pkg/list/utils/utils.go (1) (1)
IsNoValuesFoundError(8:11)
🪛 LanguageTool
cmd/markdown/atmos_list_settings_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all settings ``` $ atmos list set...
(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...ettings $ atmos list settings – List specific settings ``` $ atmos lis...
(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...list settings --query '.terraform' – Filter by stack pattern $ atmos li...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...os list settings --stack '-dev-' – Output in different formats $ atmo...
(DASH_RULE)
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list settings --format tsv – Disable Go template processing $ a...
(DASH_RULE)
[typographical] ~28-~28: Consider using an em dash in dialogues and enumerations.
Context: ...settings --process-templates=false – Disable YAML functions processing ...
(DASH_RULE)
cmd/markdown/atmos_list_metadata_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all metadata ``` $ atmos list met...
(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...etadata $ atmos list metadata – List specific metadata ``` $ atmos lis...
(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...list metadata --query '.component' – Filter by stack pattern $ atmos li...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...os list metadata --stack '-dev-' – Output in different formats $ atmo...
(DASH_RULE)
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list metadata --format tsv – Disable Go template processing $ a...
(DASH_RULE)
[typographical] ~28-~28: Consider using an em dash in dialogues and enumerations.
Context: ...metadata --process-templates=false – Disable YAML functions processing ...
(DASH_RULE)
cmd/markdown/atmos_list_vars_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all variables for a component ``` ...
(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vars <component> – List specific variables using query ```...
(DASH_RULE)
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ...t> – List specific variables using query $ atmos list vars --qu...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...ars --query .vars.tags – Filter by stack pattern $ atmos li...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...vars --stack '-dev-' – Output in different formats $ atmo...
(DASH_RULE)
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: ...list vars --format tsv – Include abstract components $ atmo...
(DASH_RULE)
[typographical] ~28-~28: Consider using an em dash in dialogues and enumerations.
Context: ...s list vars --abstract ``` – Filter by stack and specific variables ...
(DASH_RULE)
[typographical] ~33-~33: Consider using an em dash in dialogues and enumerations.
Context: ...ack '-ue2-' --query .vars.region – Disable Go template processing $ a...
(DASH_RULE)
[typographical] ~38-~38: Consider using an em dash in dialogues and enumerations.
Context: ...mponent> --process-templates=false – Disable YAML functions processing ...
(DASH_RULE)
cmd/markdown/atmos_list_values_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all values for a component ``` $ ...
(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...` $ atmos list values – List only variables for a component...
(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...t values --query .vars ``` – List settings for a specific component ...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...ery .settings --stack 'plat-ue2-*' – Include abstract components $ atmo...
(DASH_RULE)
[typographical] ~20-~20: Consider using an em dash in dialogues and enumerations.
Context: ...list values --abstract – Limit number of columns $ atmos li...
(DASH_RULE)
[uncategorized] ~21-~21: You might be missing the article “the” here.
Context: ...ues --abstract – Limit number of columns $ atmos list values <co...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: ...values --max-columns 5 – Output in different formats $ atmo...
(DASH_RULE)
[typographical] ~33-~33: Consider using an em dash in dialogues and enumerations.
Context: ...st values --format tsv ``` – Filter stacks and include abstract comp...
(DASH_RULE)
[typographical] ~38-~38: Consider using an em dash in dialogues and enumerations.
Context: ...ent> --stack '-prod-' --abstract ``` – Custom query with specific stack patter...
(DASH_RULE)
[typographical] ~43-~43: Consider using an em dash in dialogues and enumerations.
Context: ...query .vars.tags --stack '-ue2-' – Apply a custom query $ atmos list ...
(DASH_RULE)
[typographical] ~48-~48: Consider using an em dash in dialogues and enumerations.
Context: ... --query '.vars.region' – Filter by stack pattern $ atmos li...
(DASH_RULE)
[typographical] ~53-~53: Consider using an em dash in dialogues and enumerations.
Context: ...lues --stack '-ue2-' ``` – Limit the number of stacks displayed ``...
(DASH_RULE)
[typographical] ~58-~58: Consider using an em dash in dialogues and enumerations.
Context: ...values --max-columns 3 – Disable Go template processing $ a...
(DASH_RULE)
[typographical] ~63-~63: Consider using an em dash in dialogues and enumerations.
Context: ...mponent> --process-templates=false – Disable YAML functions processing ...
(DASH_RULE)
🪛 GitHub Check: golangci-lint
cmd/list_values.go
[failure] 88-88:
Error return value of (*github.com/spf13/pflag.FlagSet).Set is not checked
cmd/list_metadata.go
[failure] 53-53:
cyclomatic: function listMetadata has cyclomatic complexity 13 (> max enabled 10)
cmd/list_settings.go
[failure] 53-53:
cyclomatic: function listSettings has cyclomatic complexity 12 (> max enabled 10)
🪛 markdownlint-cli2 (0.17.2)
cmd/markdown/atmos_list_values_usage.md
2-2: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
3-3: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
7-7: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
8-8: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
12-12: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
13-13: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
17-17: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
18-18: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
22-22: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
23-23: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
27-27: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
28-28: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
29-29: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
30-30: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
31-31: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
35-35: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
36-36: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
40-40: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
41-41: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
45-45: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
46-46: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
50-50: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
51-51: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
55-55: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
56-56: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
60-60: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
61-61: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
65-65: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
66-66: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (12)
cmd/markdown/atmos_list_metadata_usage.md (2)
1-35: Consider using standard markdown formatting conventions.The documentation is clear and provides good examples, but there are some formatting considerations:
- Use em dashes (—) instead of hyphens (-) for introducing examples to align with documentation conventions
- Consider specifying language for code blocks, which improves syntax highlighting
-– List all metadata +- List all metadata+bash
$ atmos list metadataThis pattern should be applied to all examples in the file.
🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all metadata ``` $ atmos list met...(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...etadata$ atmos list metadata– List specific metadata ``` $ atmos lis...(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...list metadata --query '.component'– Filter by stack pattern$ atmos li...(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...os list metadata --stack '-dev-'– Output in different formats$ atmo...(DASH_RULE)
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list metadata --format tsv– Disable Go template processing$ a...(DASH_RULE)
[typographical] ~28-~28: Consider using an em dash in dialogues and enumerations.
Context: ...metadata --process-templates=false– Disable YAML functions processing...(DASH_RULE)
34-35: Good documentation of glob pattern support.Clear explanation of stack pattern glob matching functionality provides helpful context for users.
cmd/markdown/atmos_list_values_usage.md (2)
1-71: Consider standardizing markdown formatting.The documentation provides good examples, but for consistency and better rendering:
- Use standard markdown dashes (-) instead of en dashes (–)
- Add language specifiers to code blocks
- Consider adding "the" before "number of columns" in line 21
-– List all values for a component +- List all values for a component+bash
$ atmos list valuesThis pattern should be applied to all examples in the file.
🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all values for a component ``` $ ...(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...` $ atmos list values– List only variables for a component...(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...t values --query .vars ``` – List settings for a specific component ...(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...ery .settings --stack 'plat-ue2-*'– Include abstract components$ atmo...(DASH_RULE)
[typographical] ~20-~20: Consider using an em dash in dialogues and enumerations.
Context: ...list values --abstract– Limit number of columns$ atmos li...(DASH_RULE)
[uncategorized] ~21-~21: You might be missing the article “the” here.
Context: ...ues --abstract– Limit number of columns$ atmos list values <co...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: ...values --max-columns 5– Output in different formats$ atmo...(DASH_RULE)
[typographical] ~33-~33: Consider using an em dash in dialogues and enumerations.
Context: ...st values --format tsv ``` – Filter stacks and include abstract comp...(DASH_RULE)
[typographical] ~38-~38: Consider using an em dash in dialogues and enumerations.
Context: ...ent> --stack '-prod-' --abstract ``` – Custom query with specific stack patter...(DASH_RULE)
[typographical] ~43-~43: Consider using an em dash in dialogues and enumerations.
Context: ...query .vars.tags --stack '-ue2-'– Apply a custom query$ atmos list ...(DASH_RULE)
[typographical] ~48-~48: Consider using an em dash in dialogues and enumerations.
Context: ... --query '.vars.region'– Filter by stack pattern$ atmos li...(DASH_RULE)
[typographical] ~53-~53: Consider using an em dash in dialogues and enumerations.
Context: ...lues --stack '-ue2-' ``` – Limit the number of stacks displayed ``...(DASH_RULE)
[typographical] ~58-~58: Consider using an em dash in dialogues and enumerations.
Context: ...values --max-columns 3– Disable Go template processing$ a...(DASH_RULE)
[typographical] ~63-~63: Consider using an em dash in dialogues and enumerations.
Context: ...mponent> --process-templates=false– Disable YAML functions processing...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
2-2: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
3-3: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
7-7: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
8-8: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
12-12: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
13-13: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
17-17: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
18-18: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
22-22: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
23-23: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
27-27: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
28-28: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
29-29: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
30-30: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
31-31: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
35-35: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
36-36: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
40-40: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
41-41: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
45-45: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
46-46: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
50-50: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
51-51: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
55-55: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
56-56: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
60-60: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
61-61: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
65-65: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
66-66: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
69-71: Correct formatting of stack pattern examples.The stack pattern glob matching explanation follows the suggested format from previous reviews.
cmd/list_values_internal_test.go (1)
1-136: Well-structured test coverage for command flags and argument validation.The tests provide thorough validation of flag existence, default values, and argument validation for both
list valuesandlist varscommands. This ensures the commands behave as expected when used with different flags and arguments.cmd/list_metadata.go (4)
33-33: Good use of semantic logging.Proper use of structured logging with field-based approach rather than string concatenation.
56-59: Good structured error handling.Using custom error types with cause encapsulation is cleaner than using fmt.Errorf for dynamic errors.
105-108: Good default query handling.Setting a meaningful default query when none is provided improves user experience.
97-103: Detailed logging aids debugging.Comprehensive logging of filter parameters helps with troubleshooting when issues arise.
cmd/markdown/atmos_list_vars_usage.md (1)
44-45: No further actions required.The mention of stack pattern support aligns with previous suggestions and looks good.
cmd/list_values.go (1)
1-256: Overall implementation looks solid.The flags and error handling appear well-structured, and the
listValuesfunction logic for describing stacks and filtering results is clear.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 88-88:
Error return value of(*github.com/spf13/pflag.FlagSet).Setis not checkedcmd/markdown/atmos_list_settings_usage.md (1)
1-34: LGTM! Documentation examples are comprehensive and clear.Your usage examples are well-structured and cover all the major use cases for the command, including various formats and options. The note about glob matching is particularly helpful for users.
🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all settings ``` $ atmos list set...(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...ettings$ atmos list settings– List specific settings ``` $ atmos lis...(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ...list settings --query '.terraform'– Filter by stack pattern$ atmos li...(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...os list settings --stack '-dev-'– Output in different formats$ atmo...(DASH_RULE)
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list settings --format tsv– Disable Go template processing$ a...(DASH_RULE)
[typographical] ~28-~28: Consider using an em dash in dialogues and enumerations.
Context: ...settings --process-templates=false– Disable YAML functions processing...(DASH_RULE)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
cmd/list_metadata.go (3)
18-23: Clarify default query usage in the command description.It’s helpful to mention that
.metadatawill be queried by default. This clarifies behavior for first-time users.
82-89: Use debug-level logging for verbose details.The info-level log here may be too detailed for standard logs. Switching to
log.Debugcan keep logs cleaner for most users.
92-94: Document the default.metadataquery in help text.This practical detail helps avoid user confusion.
cmd/list_values.go (3)
36-47: Add clarity for the required component argument.Expanding the usage text to highlight that exactly one component argument is mandatory could reduce user confusion.
53-54: Assess error handling on Atmos configuration checks.After calling
checkAtmosConfig(), consider surfacing any config errors immediately to the user with a non-zero exit code.
201-203: Offer a more descriptive message for missing component names.Including actionable guidance (e.g., “Run ‘atmos list values --help’ for usage”) can help users fix their commands faster.
pkg/list/flags/flags.go (1)
24-30: Expand usage hints for added flags.Brief examples of valid inputs greatly reduce guesswork when setting these flags.
cmd/list_settings.go (2)
53-107: Consider refactoring to reduce complexity.The
listSettingsfunction has a cyclomatic complexity of 12, which exceeds the project's maximum of 10. This was flagged in a previous static analysis.Consider extracting the stack retrieval and filtering logic into separate helper functions:
func listSettings(cmd *cobra.Command) (string, error) { // Get common flags commonFlags, err := fl.GetCommonListFlags(cmd) if err != nil { return "", &errors.CommonFlagsError{Cause: err} } // Get template and function processing flags processingFlags := fl.GetProcessingFlags(cmd) if f.Format(commonFlags.Format) == f.FormatCSV && commonFlags.Delimiter == f.DefaultTSVDelimiter { commonFlags.Delimiter = f.DefaultCSVDelimiter } + + return getAndFilterSettings(commonFlags, processingFlags) +} + +func getAndFilterSettings(commonFlags *fl.CommonListFlags, processingFlags *fl.ProcessingFlags) (string, error) { // Initialize CLI config configAndStacksInfo := schema.ConfigAndStacksInfo{} atmosConfig, err := config.InitCliConfig(configAndStacksInfo, true) if err != nil { return "", &errors.InitConfigError{Cause: err} } // Get all stacks stacksMap, err := e.ExecuteDescribeStacks(atmosConfig, "", nil, nil, nil, false, processingFlags.Templates, processingFlags.Functions, false, nil) if err != nil { return "", &errors.DescribeStacksError{Cause: err} } // Log the settings query log.Info("Filtering settings", "query", commonFlags.Query, "maxColumns", commonFlags.MaxColumns, "format", commonFlags.Format, "stackPattern", commonFlags.Stack, "processTemplates", processingFlags.Templates, "processYamlFunctions", processingFlags.Functions) // Use empty query to avoid further processing since handleComponentProperties will extract the settings output, err := l.FilterAndListValues(stacksMap, &l.FilterOptions{ Component: "settings", Query: commonFlags.Query, IncludeAbstract: false, MaxColumns: commonFlags.MaxColumns, FormatStr: commonFlags.Format, Delimiter: commonFlags.Delimiter, StackPattern: commonFlags.Stack, }) if err != nil { if u.IsNoValuesFoundError(err) { return "", &errors.NoSettingsFoundError{Query: commonFlags.Query} } return "", &errors.SettingsFilteringError{Cause: err} } return output, nil
89-98: Consider setting a default query value for better usability.The code currently uses whatever query the user provides. If they don't specify one, setting a sensible default would improve usability.
// Use empty query to avoid further processing since handleComponentProperties will extract the settings + query := commonFlags.Query + if query == "" { + query = ".settings" + } + output, err := l.FilterAndListValues(stacksMap, &l.FilterOptions{ Component: "settings", Query: commonFlags.Query, - Query: query, IncludeAbstract: false, MaxColumns: commonFlags.MaxColumns, FormatStr: commonFlags.Format, Delimiter: commonFlags.Delimiter, StackPattern: commonFlags.Stack, })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/list_metadata.go(1 hunks)cmd/list_settings.go(1 hunks)cmd/list_values.go(1 hunks)pkg/list/flags/flags.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/list_settings.go (2)
Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2025-03-18T00:20:29.426Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
Learnt from: osterman
PR: cloudposse/atmos#1036
File: cmd/list_settings.go:46-50
Timestamp: 2025-03-18T00:20:29.426Z
Learning: In the Atmos project, -1 is used as a special value to denote unlimited columns in table outputs, following common software conventions.
🧬 Code Definitions (3)
cmd/list_settings.go (4)
pkg/list/errors/types.go (21) (21)
CommonFlagsError(82:84)InitConfigError(95:97)e(14:19)e(27:29)e(37:39)e(41:43)e(51:53)e(55:57)e(64:66)e(73:75)e(77:79)e(86:88)e(90:92)e(99:101)e(103:105)e(112:114)e(116:118)e(125:127)DescribeStacksError(108:110)NoSettingsFoundError(121:123)SettingsFilteringError(130:132)pkg/list/format/formatter.go (2) (2)
Format(8:8)FormatCSV(14:14)pkg/list/list_values.go (3) (3)
FilterAndListValues(58:88)FilterOptions(47:55)IsNoValuesFoundError(342:345)pkg/list/utils/utils.go (1) (1)
IsNoValuesFoundError(8:11)
cmd/list_metadata.go (4)
pkg/list/flags/flags.go (2) (2)
AddCommonListFlags(24:30)GetCommonListFlags(44:84)pkg/list/errors/types.go (19) (19)
QueryError(32:35)InitConfigError(95:97)e(14:19)e(27:29)e(37:39)e(41:43)e(51:53)e(55:57)e(64:66)e(73:75)e(77:79)e(86:88)e(90:92)e(99:101)e(103:105)e(112:114)e(116:118)e(125:127)MetadataFilteringError(69:71)pkg/list/list_values.go (3) (3)
FilterAndListValues(58:88)FilterOptions(47:55)IsNoValuesFoundError(342:345)pkg/list/utils/utils.go (1) (1)
IsNoValuesFoundError(8:11)
pkg/list/flags/flags.go (1)
pkg/list/flags.go (2) (2)
AddCommonListFlags(21:27)GetCommonListFlags(30:76)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (6)
cmd/list_metadata.go (1)
28-39: Consider returning a non-zero exit code on failure.Currently, the function logs the error and returns without explicitly signaling an error code. For CLI tools, returning a non-zero status is a common practice.
pkg/list/flags/flags.go (2)
44-84: Reevaluate logging within library-level code.Currently, the function logs internally and returns errors. Some libraries prefer returning errors unlogged to let callers decide on logging.
87-116: Good defaulting approach for template and function flags.Maintaining safe defaults like using templates and functions out-of-the-box is user-friendly.
cmd/list_settings.go (3)
46-46: LGTM - Support for unlimited columns with max-columns=-1 is implemented correctly.This implementation correctly supports using -1 as a special value to denote unlimited columns in table outputs, following common software conventions.
37-37: Code correctly uses utils.PrintMessage instead of fmt.Println.The implementation correctly follows the project's linting rules by using the provided utility function rather than fmt.Println directly.
57-57: Error handling correctly uses structured error types.The code follows best practices by using predefined structured error types instead of dynamic errors, which aligns with the project's error handling patterns.
Also applies to: 71-71, 77-77, 101-103
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/list_values.go (2)
181-185: Validate user-supplied formats.When users specify a CSV format, the delimiter logic is automatically set. It might be helpful to validate that the user-supplied delimiter is compatible with the chosen format for better error messages.
233-243: Revisit log level for filtering details.You currently log filter options at the “Info” level. Depending on the project’s logging strategy, consider lowering to “Debug” to reduce potential clutter in production logs.
pkg/list/flags/flags.go (1)
54-95: Avoid double-logging errors.You log the error and also return it wrapped. Downstream code may log the error again. This can lead to repetitive logs. Decide on a single place to log or omit the log here to avoid duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/list_values.go(1 hunks)pkg/list/flags/flags.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (4)
cmd/list_values.go (2)
44-50: Consider embedding examples from a Markdown file.These usage examples are hardcoded. The project often embeds similar examples from separate Markdown files for reusability and consistency. Reusing that pattern here would align this command’s examples with the standards used elsewhere.
86-90: Looks good.Flagging errors when setting the “query” flag is a good approach. The error handling flow is consistent.
pkg/list/flags/flags.go (2)
34-41: Looks good.The newly introduced flags match the project’s naming conventions and appear to be consistent with typical usage patterns.
97-128: LGTM!The approach for handling optional flags and falling back to defaults is solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/list/flags/flags.go (3)
48-57: Potential over-engineering.Defining a
flagGetterinterface andgetFlagsFnfunction is helpful for testing or mocking. If not strictly needed, consider simplifying. Otherwise, your approach is well-structured.
59-100: Unify repetitive flag retrieval logic.Retrieving and logging errors for each flag is almost identical. To reduce duplication, create a helper function that retrieves a flag, logs on error, and returns a typed error. This increases maintainability.
Here is a possible snippet to unify the logic:
-func GetCommonListFlags(cmd *cobra.Command) (*CommonFlags, error) { - flags := getFlagsFn(cmd) - - format, err := flags.GetString("format") - if err != nil { - log.Error("failed to retrieve format flag", "error", err) - return nil, fmt.Errorf(ErrFmtWrapErr, ErrFetchingFormat, err) - } - ... -} +func getStringFlag( + flags flagGetter, + flagName string, + typedErr error, +) (string, error) { + val, err := flags.GetString(flagName) + if err != nil { + log.Error("failed to retrieve flag", "flag", flagName, "error", err) + return "", fmt.Errorf(ErrFmtWrapErr, typedErr, err) + } + return val, nil +} + +func GetCommonListFlags(cmd *cobra.Command) (*CommonFlags, error) { + flags := getFlagsFn(cmd) + + format, err := getStringFlag(flags, "format", ErrFetchingFormat) + if err != nil { + return nil, err + } + ... +}
102-133: Defaulting to true on errors might mask issues.When failing to retrieve
process-templatesorprocess-functions, defaulting totrueis forgiving for users, but could hide user misconfiguration. Consider returning an error or at least logging a warning if you want stricter behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/flags/flags.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/list/flags/flags.go (3)
pkg/list/format/formatter.go (1) (1)
Format(8:8)pkg/list/flags.go (2) (2)
AddCommonListFlags(21:27)GetCommonListFlags(30:76)cmd/list_values.go (1) (1)
ErrFmtWrapErr(30:30)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/list/flags/flags.go (4)
1-22: Well-defined error handling constants.The approach of combining typed errors (e.g.,
ErrFetchingFormat) with a generic error format template (ErrFmtWrapErr) is concise and clear for debugging. No concerns here.
24-31: Struct captures all required flags.The
CommonFlagsstruct is well-structured and straightforward. No issues noted with naming or usage.
33-37: ProcessingFlags usage is appropriate.The
ProcessingFlagsstruct is minimal and readable. It’s also flexible enough for further expansion if additional processing flags are needed.
39-46: Check usage of PersistentFlags if only local scope is intended.When attaching flags via
PersistentFlags(), all subcommands inherit them. This is likely the intended design, but confirm that you expect these flags to be accessible to subcommands.Would you like to keep these flags globally accessible or switch to local command flags if not necessary?
aknysh
added
no-release
|
@Cerebrovinny i see a few issues: (tested in the folder
INFO Filtering values component=c1 query=.vars includeAbstract=false maxColumns=0 format="" stackPattern="" processTemplates=true processYamlFunctions=true
DEBU query returned nil stack=prod query=.vars
DEBU query returned nil stack=test query=.vars
DEBU query returned nil stack=staging query=.vars
DEBU query returned nil stack=sandbox query=.vars
DEBU query returned nil stack=dev query=.vars
ERRO no values found for component with query component=c1 query=.vars
INFO Filtering metadata query="" maxColumns=0 format="" stackPattern="" processTemplates=true processYamlFunctions=true
DEBU no values found stack=sandbox component="" yq_expression=".components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars" error="EvaluateYqExpression: failed to evaluate YQ expression '.components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars': bad expression, please check expression syntax"
DEBU no values found stack=dev component="" yq_expression=".components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars" error="EvaluateYqExpression: failed to evaluate YQ expression '.components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars': bad expression, please check expression syntax"
DEBU no values found stack=prod component="" yq_expression=".components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars" error="EvaluateYqExpression: failed to evaluate YQ expression '.components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars': bad expression, please check expression syntax"
DEBU no values found stack=staging component="" yq_expression=".components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars" error="EvaluateYqExpression: failed to evaluate YQ expression '.components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars': bad expression, please check expression syntax"
DEBU no values found stack=test component="" yq_expression=".components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars" error="EvaluateYqExpression: failed to evaluate YQ expression '.components.terraform. | select(has(\"abstract\") == false or .abstract == false) | .vars': bad expression, please check expression syntax"
ERRO failed to list metadata error="no metadata found in any stacks with query '.metadata'"Please look into the issues |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/list/flags/flags.go (3)
10-23: Good use of typed errors and a shared wrap format.
They clearly communicate the issue source and simplify error handling. Consider adding a short doc comment describing when to use each error constant, so future contributors quickly know their intent.
24-37: Consider adding field-level doc comments.
Providing brief descriptions for each field inCommonFlagsandProcessingFlagswould clarify their purpose and reduce confusion.
59-100: Check for double-logging.
Logging errors before returning them can lead to duplicate messages in some flows. Consider returning the wrapped error without logging, or use a custom log level for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/flags/flags.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/list/flags/flags.go (3)
39-46: Concise and clean flag definitions.
This block is well-structured, and the short usage strings help users quickly grasp each flag’s purpose. Good work.
48-57: Purposeful use of the interface for testing.
TheflagGetterinterface andgetFlagsFnprovide a seamless way to mock flags. This approach promotes testability and is well-reasoned.
102-133: Good defaults for processing flags.
The fallback approach is sensible. If you need more advanced validations or conditional logic, handle them similarly to the typed error pattern used earlier. Overall, looks solid.
What
atmos list valuesCLI command to list component values across stacksatmos list metadataCLI command to list component metadata across stacksatmos list settingsCLI command to list component settings across stacksWhy
Examples
atmos list values
atmos list metadata
atmos list settings
Evidences:
Evidence Updated at 2025-03-17
Evidence Updated at 2025-02-24
Table too wide
references
Summary by CodeRabbit
New Features
Documentation
atmos list values,atmos list vars,atmos list metadata, andatmos list settingscommands, detailing filtering options and format selection for enhanced comparison across stacks.atmos list workflowscommand, providing usage instructions and examples.Tests
Chores