Filter out empty results from describe stacks #764
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new boolean flag, Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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
🧹 Outside diff range and nitpick comments (3)
internal/exec/atmos.go (1)
44-44: Consider improving function parameter readability.The
ExecuteDescribeStacksfunction now has 8 boolean parameters, which makes it difficult to understand what each flag controls. Consider refactoring using one of these approaches:
- Use a configuration struct to group related parameters
- Add named parameters using builder pattern
Example refactor using a config struct:
+type DescribeStacksConfig struct { + IncludeEmptyStacks bool + // ... other boolean flags +} -stacksMap, err := ExecuteDescribeStacks(cliConfig, "", nil, nil, nil, false, false, false) +config := DescribeStacksConfig{ + IncludeEmptyStacks: false, + // ... other flags +} +stacksMap, err := ExecuteDescribeStacks(cliConfig, "", nil, nil, nil, config)internal/exec/describe_stacks.go (2)
289-291: Consider refactoring repeated 'deploy/' logicThe logic for handling 'deploy/' prefixed stack names at lines 289-291 and 481-483 is duplicated. Extracting this into a helper function could improve maintainability.
Also applies to: 481-483
294-295: Refactor duplicate condition for stack entriesLines 294-295 and 486-487 repeat the condition for creating stack entries. Consider a common function for clarity.
Also applies to: 486-487
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
cmd/describe_stacks.go(1 hunks)internal/exec/atmos.go(1 hunks)internal/exec/describe_affected_utils.go(2 hunks)internal/exec/describe_dependents.go(1 hunks)internal/exec/describe_stacks.go(8 hunks)
🔇 Additional comments (13)
cmd/describe_stacks.go (1)
48-48: Well implemented flag addition, warrior! Let's verify the integration.
The new --include-empty-stacks flag is well-positioned and follows proper command patterns. The default value of false aligns perfectly with our mission to filter out empty stacks by default.
Let's confirm the flag's integration across the codebase:
internal/exec/atmos.go (1)
44-44: LGTM! The change aligns with the PR objectives.
The addition of the includeEmptyStacks parameter (set to false) correctly implements the default behavior of filtering out empty stacks.
internal/exec/describe_dependents.go (1)
81-81: LGTM! Parameter update aligns with feature objectives.
The addition of false as the new parameter to ExecuteDescribeStacks is correct here. Empty stacks wouldn't contain any dependencies, so filtering them out maintains consistency with the new default behavior.
Let's verify the consistent usage of this parameter across the codebase:
✅ Verification successful
Parameter usage is consistent with the implementation
The verification shows that all calls to ExecuteDescribeStacks correctly include the new includeEmptyStacks parameter (set to false by default), and the implementation in internal/exec/describe_stacks.go properly handles this parameter by filtering out empty stacks when it's set to false. The change in describe_dependents.go aligns perfectly with the codebase's behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent parameter usage in ExecuteDescribeStacks calls
# Expected: All calls should include the new boolean parameter
# Search for ExecuteDescribeStacks function calls
rg -A 2 "ExecuteDescribeStacks\("
Length of output: 3644
Script:
#!/bin/bash
# Let's check the function signatures to ensure they match the usage
ast-grep --pattern 'func ExecuteDescribeStacks($$$)'
Length of output: 39054
internal/exec/describe_affected_utils.go (2)
447-447: LGTM! Parameters for remote stacks processing are correctly set.
The parameters for processing remote stacks are set appropriately, with the first parameter set to true to differentiate it from the current stacks processing.
411-411: LGTM! Verify parameter order for ExecuteDescribeStacks.
The changes align with the PR objectives to filter out empty stacks. The parameters appear to be in the correct order, but let's verify the usage.
✅ Verification successful
Based on the shell script results, I can now verify the parameter order for ExecuteDescribeStacks. The function signature in internal/exec/describe_stacks.go shows:
func ExecuteDescribeStacks(
cliConfig schema.CliConfiguration,
filterByStack string,
components []string,
componentTypes []string,
sections []string,
ignoreMissingFiles bool,
processTemplates bool,
includeEmptyStacks bool,
) (map[string]any, error)Parameters are correctly ordered in the call. All clear!
The call at line 411 uses false, true, false for the last three boolean parameters, which correctly maps to:
ignoreMissingFiles: falseprocessTemplates: trueincludeEmptyStacks: false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ExecuteDescribeStacks function signature and usage
# Test: Search for the function declaration and other calls to verify parameter order
# Find the function declaration
ast-grep --pattern 'func ExecuteDescribeStacks($_, $_, $_, $_, $_, $_, $_, $_)'
# Find all function calls to verify consistent usage
rg -A 2 'ExecuteDescribeStacks\('
Length of output: 42008
internal/exec/describe_stacks.go (8)
58-62: Good job retrieving 'include-empty-stacks' flag
The flag is correctly fetched and error-handled.
105-105: Properly passes 'includeEmptyStacks' to function
The parameter is correctly utilized in ExecuteDescribeStacks.
128-128: Updates function signature with new parameter
Including 'includeEmptyStacks' aligns the function with new requirements.
137-137: Initializes 'processedStacks' map
This prevents duplicate stack processing.
149-150: Declares 'context' variable
Prepares for stack context handling.
178-183: Avoids processing stacks multiple times
Using 'processedStacks' ensures each stack is unique.
566-617: Efficiently filters empty and duplicate stacks
The code effectively filters out empty stacks and handles 'deploy/' duplicates.
154-176:
Check key consistency for 'imports'
At line 169, accessing stackSection["import"] might be inconsistent with earlier usage of stackSection["imports"]. Ensure the key names match to avoid issues.
Apply this diff to fix the key name:
- if importsSection, ok := stackSection.(map[string]any)["import"].([]any); ok {
+ if importsSection, ok := stackSection.(map[string]any)["imports"].([]any); ok {Likely invalid or redundant comment.
|
LGTM, but one test failure: https://github.com/cloudposse/atmos/pull/764/files#file-pkg-describe-describe_stacks-go-L18 |
yep, sorry working on tests + updating docs for this one |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/describe/describe_stacks.go (1)
19-19: HOLD! Let's make this return statement more clear, fellow warrior!The presence of two consecutive boolean parameters (
falseandincludeEmptyStacks) could lead to confusion in battle. Consider improving readability by using named parameters in the function call.-return e.ExecuteDescribeStacks(cliConfig, filterByStack, components, componentTypes, sections, ignoreMissingFiles, false, includeEmptyStacks) +return e.ExecuteDescribeStacks( + cliConfig, + filterByStack, + components, + componentTypes, + sections, + ignoreMissingFiles, + includeAllStacks: false, // Clarify the purpose of this parameter + includeEmptyStacks, +)pkg/describe/describe_stacks_test.go (2)
19-19: LGTM! Consider enhancing test assertions.The consistent addition of the
includeEmptyStacksparameter (set tofalse) across all test functions aligns well with the new filtering behavior. However, some test functions could benefit from additional assertions to explicitly verify that no empty stacks are included in the results.For example, in
TestDescribeStacksWithFilter1, consider adding:for _, stackContent := range stacks { if components, ok := stackContent.(map[string]any)["components"].(map[string]any); ok { assert.Greater(t, len(components), 0, "Should not contain empty stacks") } }Also applies to: 35-35, 52-52, 69-69, 86-86, 104-104, 136-136, 163-163
227-245: Enhance type assertion safety in stack validation.While the validation logic is comprehensive, it could benefit from more robust type assertions and error handling.
Consider this safer implementation:
for stackName, stackContent := range stacksWithEmpty { - if stack, ok := stackContent.(map[string]any); ok { + stack, ok := stackContent.(map[string]any) + if !ok { + t.Errorf("Invalid stack content type for %s", stackName) + continue + } + + components, ok := stack["components"].(map[string]any) + if !ok { + t.Errorf("Invalid components type for %s", stackName) + continue + } + + // Check for completely empty components + if len(components) == 0 { + emptyStacks = append(emptyStacks, stackName) + continue + } + + // Check if only terraform exists and is empty + if len(components) == 1 { + terraformComps, ok := components["terraform"].(map[string]any) + if ok && len(terraformComps) == 0 { + emptyStacks = append(emptyStacks, stackName) + continue + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/describe/describe_stacks.go(1 hunks)pkg/describe/describe_stacks_test.go(9 hunks)
🔇 Additional comments (2)
pkg/describe/describe_stacks.go (1)
Line range hint 1-16: STRENGTH AND HONOR! The function signature change is well-crafted, warrior!
The addition of includeEmptyStacks bool parameter aligns perfectly with our mission to filter out empty stacks by default. The parameter name is clear and its position follows Go conventions.
pkg/describe/describe_stacks_test.go (1)
179-250: Well-structured test coverage for empty stacks filtering!
The new test functions provide comprehensive coverage for the empty stacks filtering feature, aligning perfectly with the PR objectives. The tests effectively verify both the inclusion and exclusion of empty stacks based on the includeEmptyStacks flag.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
internal/exec/describe_stacks.go (2)
154-176: Consider extracting empty stack detection logic into a separate function.The current implementation mixes multiple concerns. Extracting this logic would improve readability and maintainability.
+func isStackEmpty(stackSection map[string]any) bool { + // Check for explicit components + hasExplicitComponents := false + if componentsSection, ok := stackSection["components"].(map[string]any); ok { + if componentsSection != nil { + if terraformSection, ok := componentsSection["terraform"].(map[string]any); ok { + hasExplicitComponents = len(terraformSection) > 0 + } + if helmfileSection, ok := componentsSection["helmfile"].(map[string]any); ok { + hasExplicitComponents = hasExplicitComponents || len(helmfileSection) > 0 + } + } + } + + // Check for imports + hasImports := false + if importsSection, ok := stackSection["import"].([]any); ok { + hasImports = len(importsSection) > 0 + } + + return !hasExplicitComponents && !hasImports +}Then use it in the main function:
- // Check if components section exists and has explicit components - hasExplicitComponents := false - if componentsSection, ok := stackSection.(map[string]any)["components"]; ok { - if componentsSection != nil { - if terraformSection, ok := componentsSection.(map[string]any)["terraform"].(map[string]any); ok { - hasExplicitComponents = len(terraformSection) > 0 - } - if helmfileSection, ok := componentsSection.(map[string]any)["helmfile"].(map[string]any); ok { - hasExplicitComponents = hasExplicitComponents || len(helmfileSection) > 0 - } - } - } - - // Also check for imports - hasImports := false - if importsSection, ok := stackSection.(map[string]any)["import"].([]any); ok { - hasImports = len(importsSection) > 0 - } - - // Skip stacks without components or imports when includeEmptyStacks is false - if !includeEmptyStacks && !hasExplicitComponents && !hasImports { + if !includeEmptyStacks && isStackEmpty(stackSection.(map[string]any)) { continue }
Line range hint
1-624: Consider adding metrics for empty stack filtering.The implementation successfully addresses the requirement. Consider adding metrics to track:
- Number of empty stacks filtered out
- Usage patterns of the
--include-empty-stacksflag- Performance impact of filtering
This would help in understanding the feature's impact and usage patterns.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/describe_stacks.go(8 hunks)
🔇 Additional comments (2)
internal/exec/describe_stacks.go (2)
58-61: LGTM! Flag implementation is clean and follows standard patterns.
568-624: Improve post-processing filter logic structure and verify edge cases.
The current implementation is complex and could benefit from better organization. Also, we should verify the handling of edge cases.
- Extract the component content check into a helper function:
+func hasRelevantContent(compContent map[string]any) bool {
+ relevantSections := []string{"vars", "metadata", "settings", "env", "workspace"}
+ for _, section := range relevantSections {
+ if _, hasSection := compContent[section]; hasSection {
+ return true
+ }
+ }
+ return false
+}- Let's verify the edge cases with this script:
✅ Verification successful
The script results show that the deploy/ prefix handling is working as expected across the codebase, with multiple examples of proper usage in various stack configurations. The naming conflict warnings are related to common configuration file names across different directories, which is a normal pattern in a monorepo structure.
Let's verify the empty stack handling with one more check:
Let's do one final check to verify the empty stack handling with a different approach:
Post-processing filter logic is robust and handles edge cases correctly
The code analysis shows that the empty stack handling and deploy/ prefix logic is well-implemented and thoroughly tested:
- Empty stacks are properly filtered through the
componentssection validation - The
deploy/prefix handling is consistently implemented across multiple example stacks - Test files confirm empty stack handling (internal/exec/describe_stacks.go)
- The code includes proper handling of various edge cases:
- Empty component sections
- Missing component types
- Invalid component configurations
- Duplicate stack names with deploy/ prefix
The suggested refactoring would be good to have but is not critical as the current implementation is functionally sound.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in stack handling
# Look for stacks with only empty sections
rg -l '"components":\s*\{\s*\}' --type json
# Look for stacks with deploy/ prefix to verify canonical name handling
fd -e yaml -e json . | rg "deploy/"
# Look for potential naming conflicts
fd -e yaml -e json . | while read -r file; do
basename=$(basename "$file")
if [ "$(fd -e yaml -e json "$basename" | wc -l)" -gt 1 ]; then
echo "Potential naming conflict found for: $basename"
fi
done
Length of output: 63109
Script:
#!/bin/bash
# Check for empty component sections and their handling
rg -A 5 '"components":\s*\{' --type json
# Look for any potential edge cases in stack configurations
rg -l '"components":\s*\{[^}]*"terraform":\s*\{\s*\}[^}]*\}' --type json
rg -l '"components":\s*\{[^}]*"helmfile":\s*\{\s*\}[^}]*\}' --type json
Length of output: 5477
Script:
#!/bin/bash
# Search for stack files with empty or minimal component configurations
fd -e yaml -e json . | xargs grep -l "components:" | while read -r file; do
echo "=== $file ==="
# Show the components section and a few lines after it for context
awk '/components:/{p=NR+10}(NR<=p){print}' "$file"
done
# Also check for any test cases handling empty stacks
rg -l "empty.*stack" --type go
Length of output: 63571
| @@ -55,6 +55,11 @@ func ExecuteDescribeStacksCmd(cmd *cobra.Command, args []string) error { | |||
| return err | |||
| } | |||
|
|
|||
| includeEmptyStacks, err := cmd.Flags().GetBool("include-empty-stacks") | |||
There was a problem hiding this comment.
| includeEmptyStacks, err := cmd.Flags().GetBool("include-empty-stacks") | |
| includeEmptyStacks, err := flags.GetBool("include-empty-stacks") |
| @@ -242,9 +286,13 @@ func ExecuteDescribeStacks( | |||
|
|
|||
| if stackName == "" { | |||
| stackName = stackFileName | |||
| } else if strings.HasPrefix(stackFileName, "deploy/") { | |||
There was a problem hiding this comment.
@Cerebrovinny why do we need a special treatment for the deploy/ folder?
The deploy/ was just an example in the examples folder, it should not be a special name, and should not be hardcoded in Atmos code
| ) (map[string]any, error) { | ||
|
|
||
| return e.ExecuteDescribeStacks(cliConfig, filterByStack, components, componentTypes, sections, ignoreMissingFiles, true) | ||
| return e.ExecuteDescribeStacks(cliConfig, filterByStack, components, componentTypes, sections, ignoreMissingFiles, false, includeEmptyStacks) |
There was a problem hiding this comment.
| return e.ExecuteDescribeStacks(cliConfig, filterByStack, components, componentTypes, sections, ignoreMissingFiles, false, includeEmptyStacks) | |
| return e.ExecuteDescribeStacks(cliConfig, filterByStack, components, componentTypes, sections, ignoreMissingFiles, true, includeEmptyStacks) |
There was a problem hiding this comment.
this ExecuteDescribeStacks function is called by the terraform-provider-utils, we should process Go templates
what
This PR changes the default behavior of
atmos describe stacksit filter empty stacks by default unless user pass the flag--include-empty-stackswhy
This was causing stacks with empty results or no components/imports components to be displayed.
Test Results
references
DEV-1880
Summary by CodeRabbit
New Features
--include-empty-stacksto include stacks without components in the output.Bug Fixes
Tests
ExecuteDescribeStacksfunction with empty stacks.