Fix: nested components for list pkg #1225
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
+ Coverage 33.56% 33.69% +0.12%
==========================================
Files 226 226
Lines 24219 24289 +70
==========================================
+ Hits 8130 8185 +55
- Misses 14875 14882 +7
- Partials 1214 1222 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 Walkthrough""" WalkthroughThe changes refactor the logic for extracting and querying component values from stacks, introducing support for a new component type Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AtmosCLI
participant ListModule
User->>AtmosCLI: Run 'atmos list vars/values <component> --stack <stack>'
AtmosCLI->>ListModule: Check if component exists across all types using full path
ListModule-->>AtmosCLI: Return existence result
alt Component exists
AtmosCLI->>ListModule: Determine component type (terraform or helmfile)
ListModule->>ListModule: Build YQ expression based on component type and filters
ListModule->>ListModule: Execute query on stack data
ListModule-->>AtmosCLI: Return extracted values
AtmosCLI-->>User: Output vars/values
else Component does not exist
AtmosCLI-->>User: Error: component does not exist
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🔭 Outside diff range comments (1)
pkg/list/list_values.go (1)
174-176: 🛠️ Refactor suggestionConsider consistent handling of component paths throughout the code.
While line 167 was updated to handle nested components correctly, other instances of
getComponentNameFromPathare still being used in this file. These might need similar updates to ensure consistent handling of nested components in all cases.Consider applying the same pattern to the remaining occurrences where
getComponentNameFromPathis used for component filtering:- componentName := getComponentNameFromPath(componentFilter) - return fmt.Sprintf(".components.%s.%s", KeyTerraform, componentName) + return fmt.Sprintf(".components.%s.\"%s\"", KeyTerraform, componentFilter)Also applies to: 180-183, 188-191
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/list/list_values.go(1 hunks)pkg/list/utils/utils.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/list/utils/utils.go
[warning] 58-58: pkg/list/utils/utils.go#L58
Added line #L58 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
![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: 3
🧹 Nitpick comments (1)
pkg/list/list_values.go (1)
241-262: Prefer wrapped sentinel errors instead of dynamicfmt.ErrorfStatic analysis rightfully points out the dynamic errors here. Defining sentinel errors (
var ErrComponentNotFound = errors.New(...)) and wrapping them (errors.Wrap) provides stronger typing and makes error handling downstream simpler.Not urgent, but adopting this pattern keeps the codebase consistent with existing lint rules.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 243-243:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("targetComponentName cannot be empty")"
[failure] 248-248:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("components section not found in stack")"
[failure] 261-261:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("component '%s' not found in terraform or helmfile sections", targetComponentName)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_values.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
pkg/list/list_values.go
[failure] 194-194:
argument-limit: maximum number of arguments per function exceeded; max 5 but got 7
[failure] 243-243:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("targetComponentName cannot be empty")"
[failure] 248-248:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("components section not found in stack")"
[failure] 261-261:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("component '%s' not found in terraform or helmfile sections", targetComponentName)"
⏰ 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
♻️ Duplicate comments (3)
pkg/list/list_values.go (3)
302-304:⚠️ Potential issueComponent path is lost in regular component branch.
Similar to past review comments, the regular component branch still strips the path from nested components with
getComponentNameFromPath(), which will break nested component support.- componentName := getComponentNameFromPath(component) - return buildComponentYqExpression(componentName, includeAbstract, componentType) + // Use full component path to support nested components + return fmt.Sprintf(".components.%s.\"%s\"", componentType, component) + + (includeAbstract ? "" : fmt.Sprintf(" | select(has(\"%s\") == false or %s%s == false)", + KeyAbstract, DotChar, KeyAbstract)) + + fmt.Sprintf(" | %s%s", DotChar, KeyVars)
307-313:⚠️ Potential issueSettings expression still loses nested paths.
The settings expression still uses
getComponentNameFromPath()which strips the parent path from nested components and doesn't wrap the component in quotes.func buildSettingsExpression(componentFilter, componentType string) string { if componentFilter != "" { - componentName := getComponentNameFromPath(componentFilter) - return fmt.Sprintf(".components.%s.%s", componentType, componentName) + // Keep full path and quote it for nested components + return fmt.Sprintf(".components.%s.\"%s\".%s", componentType, componentFilter, KeySettings) } - return "select(.settings // .terraform.settings // .components.terraform.*.settings)" + // Generic fallback that works for both component types + return fmt.Sprintf("select(.settings // .%s.settings // .components.%s.*.%s)", + componentType, componentType, KeySettings) }
315-321:⚠️ Potential issueMetadata expression needs the same fix for nested paths.
The metadata expression has the same issue as the settings expression - it doesn't preserve nested paths.
func buildMetadataExpression(componentFilter, componentType string) string { if componentFilter != "" { - componentName := getComponentNameFromPath(componentFilter) - return fmt.Sprintf(".components.%s.%s", componentType, componentName) + // Keep full path and quote it for nested components + return fmt.Sprintf(".components.%s.\"%s\".%s", componentType, componentFilter, KeyMetadata) } return DotChar + KeyMetadata }
🧹 Nitpick comments (1)
pkg/list/list_values.go (1)
200-209: Pass QueryParams by pointer to fix static analysis warning.The
QueryParamsstruct helps organize parameters nicely, but the static analyzer is flagging that it's large (96 bytes) when passed by value.-func executeQueryForStack(params QueryParams) (interface{}, error) { +func executeQueryForStack(params *QueryParams) (interface{}, error) {Also update the call site:
-value, err := executeQueryForStack(params) +value, err := executeQueryForStack(¶ms)Also applies to: 211-244
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_values.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
pkg/list/list_values.go
[failure] 211-211:
hugeParam: params is heavy (96 bytes); consider passing it by pointer
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/list/list_values.go (8)
19-23: Good use of predefined error variables.Using predefined error variables with
errors.New()improves error handling consistency and allows for better error checking throughout the codebase.
30-31: Appropriate addition of helmfile constant.Adding the
KeyHelmfileconstant properly supports the new helmfile component type alongside terraform, enabling consistent handling of both component types.
126-148: Good refactoring of component extraction logic.The extraction logic has been nicely modularized from a likely monolithic function into smaller, specialized functions with clear responsibilities. This makes the code more maintainable and easier to understand.
159-184: Appropriate component type detection and error handling.The new function properly extracts component values from a single stack with improved component type detection and graceful error handling by returning nil on errors rather than failing the entire operation.
259-289: Good component type detection implementation.The new functions properly check for components in both terraform and helmfile sections, returning appropriate errors when components aren't found. This enables proper handling of nested components across different component types.
291-294: Properly handling nested component paths for filtered components.The direct use of the full component path in quotes ensures that nested components like
apis/weatherwork correctly with YQ expressions.
342-358: Clean extraction logic for settings data.The function properly handles special case extraction for settings data, reducing duplication and improving maintainability.
567-572: Simplified result formatting logic.The function has been refactored to remove unnecessary parameters and simplify the logic, which makes it more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/list/list_values.go (3)
186-198: Default component type should be reconsidered.When the target component is empty, the function defaults to Terraform. Now that we support multiple component types, we should revisit this default.
Consider returning an empty string or detecting the component type dynamically:
func detectComponentTypeInStack(stackMap map[string]interface{}, targetComponent, stackName string) string { if targetComponent == "" { - return KeyTerraform + // Either return empty string or detect based on what's available in the stack + return detectDefaultComponentType(stackMap) } ... } +// detectDefaultComponentType determines which component type to use as default +// based on what sections are available in the stack +func detectDefaultComponentType(stackMap map[string]interface{}) string { + components, ok := stackMap[KeyComponents].(map[string]interface{}) + if !ok { + return KeyTerraform // Fallback to Terraform for backward compatibility + } + + // Check if either section exists and has components + _, hasTerraform := components[KeyTerraform].(map[string]interface{}) + _, hasHelmfile := components[KeyHelmfile].(map[string]interface{}) + + if hasTerraform { + return KeyTerraform + } else if hasHelmfile { + return KeyHelmfile + } + + return KeyTerraform // Default for backward compatibility +}
259-280: Consider using errors.Wrap instead of fmt.Errorf.Using errors.Wrap would be more consistent with other error handling in the codebase.
- return "", fmt.Errorf("%w: %s", ErrComponentNotFoundInSections, targetComponentName) + return "", errors.Wrapf(ErrComponentNotFoundInSections, "%s", targetComponentName)
283-289: Consider adding logging to isComponentInSection.Adding debug logging would help with troubleshooting nested component issues.
func isComponentInSection(components map[string]interface{}, sectionKey, componentName string) bool { section, ok := components[sectionKey].(map[string]interface{}) if !ok { + log.Debug("Section not found", "section", sectionKey) return false } _, exists := section[componentName] + log.Debug("Component check", "section", sectionKey, "component", componentName, "exists", exists) return exists }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_values.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
pkg/list/list_values.go
[failure] 211-211:
hugeParam: params is heavy (96 bytes); consider passing it by pointer
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/list/list_values.go (6)
20-23: Good addition of structured error variables.Adding dedicated error variables makes the code more maintainable and helps with error handling consistency.
30-31: Excellent addition of Helmfile support.Adding the KeyHelmfile constant enables handling both Terraform and Helmfile components, which is essential for nested component support.
126-148: Clean refactoring of extraction logic.Breaking down the component extraction into smaller, focused functions improves readability and maintainability.
201-209: Good struct creation to simplify function signatures.Creating a QueryParams struct is cleaner than passing many individual parameters.
269-277: Good addition of component type detection.Now properly checks for components in both terraform and helmfile sections.
329-338: Good implementation of nested component support.Properly quoting the component path prevents issues with nested paths containing slashes.
what
Enabled atmos list vars and atmos list values to correctly display information for nested components (e.g., apis/weather, eks/cluster).
why
previously we assume and extract only the basename of nested components
These changes ensure the full component path is used for both existence checks and data querying.
references
atmos list varsandatmos list valuesdoes not work on nested components #1220Summary by CodeRabbit