[describe-stacks] Fix: switch --component-types to StringSlice to match []string and support multi-values#1432
Conversation
--component-types a String flag (CSV-supported) and normalize to []string downstream
--component-types a String flag (CSV-supported) and normalize to []string downstream
📝 WalkthroughWalkthroughChanged the CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI user
participant CLI as cmd/describe_stacks.go
participant Flags as pflag.FlagSet
participant Binder as setCliArgsForDescribeStackCli
participant Exec as exec.DescribeStacksArgs
Note over User,CLI: User runs `atmos describe-stacks --component-types=terraform,helmfile`
User->>CLI: invoke command
CLI->>Flags: define StringSlice "component-types"
Flags->>CLI: parse args
CLI->>Binder: call setCliArgsForDescribeStackCli(flags)
Binder->>Flags: GetStringSlice("component-types")
Flags-->>Binder: ["terraform","helmfile"]
Binder->>Exec: set ComponentTypes = ["terraform","helmfile"]
Note right of Exec: Args populated with slice
sequenceDiagram
participant Caller as Merge caller
participant Merge as pkg/merge.Merge
participant Options as local strategy logic
participant Merger as MergeWithOptions
Caller->>Merge: Merge(atmosConfig=nil, inputs)
Merge->>Options: compute strategy (nil -> "replace")
Options-->>Merge: strategy = "replace"
Merge->>Merger: MergeWithOptions(inputs, append=false, sliceDeepCopy=false)
Merger-->>Caller: merged result (replace semantics)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/describe_stacks.go(1 hunks)cmd/describe_stacks_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
cmd/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/*.go: Implement each Cobra command in a separate file under the cmd/ directory
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in command help
Provide meaningful feedback to users in command implementation
Include progress indicators for long-running operations
Provide clear error messages to users
Include troubleshooting hints when appropriate
Log detailed errors for debugging
Files:
cmd/describe_stacks_test.gocmd/describe_stacks.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
cmd/describe_stacks_test.gocmd/describe_stacks.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
Files:
cmd/describe_stacks_test.go
🧠 Learnings (1)
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
cmd/describe_stacks_test.go
🧬 Code graph analysis (1)
cmd/describe_stacks_test.go (1)
internal/exec/describe_stacks.go (1)
DescribeStacksArgs(18-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1432 +/- ##
=======================================
Coverage 55.61% 55.62%
=======================================
Files 273 273
Lines 28620 28621 +1
=======================================
+ Hits 15918 15919 +1
+ Misses 10918 10916 -2
- Partials 1784 1786 +2
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:
|
|
For some reason, we're getting this error on |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/merge/merge.go (2)
86-93: Polish the invalid-strategy error (no newline).Drop the embedded newline to keep errors single-line and easier to wrap/log.
- return nil, fmt.Errorf("invalid Atmos manifests list merge strategy '%s'.\n"+ - "Supported list merge strategies are: %s.", + return nil, fmt.Errorf("invalid Atmos manifests list merge strategy %q. Supported list merge strategies are: %s.", strategy, fmt.Sprintf("%s, %s, %s", ListMergeStrategyReplace, ListMergeStrategyAppend, ListMergeStrategyMerge))
98-104: Tiny readability tweak: prefer switch.A switch expresses the three discrete strategies a bit clearer.
- if strategy == ListMergeStrategyMerge { - sliceDeepCopy = true - } else if strategy == ListMergeStrategyAppend { - appendSlice = true - } + switch strategy { + case ListMergeStrategyMerge: + sliceDeepCopy = true + case ListMergeStrategyAppend: + appendSlice = true + }pkg/merge/merge_test.go (1)
26-38: Add an error-path test for invalid strategy.Covers negative case per testing guidelines.
func TestMerge_InvalidStrategy_ReturnsError(t *testing.T) { atmosConfig := schema.AtmosConfiguration{ Settings: schema.AtmosSettings{ListMergeStrategy: "bogus"}, } _, err := Merge(&atmosConfig, []map[string]any{{"list": []string{"1"}}}) assert.Error(t, err) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/merge/merge.go(1 hunks)pkg/merge/merge_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
Files:
pkg/merge/merge_test.gopkg/merge/merge.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
Files:
pkg/merge/merge_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: aknysh
PR: cloudposse/atmos#1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
PR: cloudposse/atmos#1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/merge/merge.go
🧬 Code graph analysis (1)
pkg/merge/merge_test.go (1)
pkg/merge/merge.go (1)
Merge(77-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/merge/merge.go (1)
81-84: Nil-safe default eliminates the panic.Initializing strategy to replace and guarding atmosConfig prevents the nil deref reported in tests. LGTM.
pkg/merge/merge_test.go (1)
26-38: Good guardrail for nil atmosConfig.Test clearly asserts default-to-replace and prevents regressions. Nice.
|
These changes were released in v1.189.0-test.0. |
Resolve merge conflicts and integrate latest main branch changes: ✅ **errors/errors.go**: Added ErrMerge error for improved error handling ✅ **pkg/merge/merge.go**: Updated merge logic with better error wrapping and strategy handling ✅ **pkg/merge/merge_test.go**: Enhanced tests with proper error verification ✅ **cmd/describe_stacks.go**: Updated component types handling to support multi-values ✅ **cmd/describe_stacks_test.go**: Updated tests for new describe stacks functionality ✅ **website/docs/**: Fixed sample stack configuration All conflicts resolved in favor of main branch improvements while preserving our gotcha tool implementation and documentation changes. Merged commits from main: - 5e2f1c3 fix: improve error handling and code quality in merge package (#1440) - fd1399a [describe-stacks] Fix: switch --component-types to StringSlice (#1432) - c5c97fb Fix sample stacks/deploy/dev.yaml in documentation (#1434)
…ch []string and support multi-values (#1432) * fix(describe-stacks): register --component-types as StringSlice to match []string * [merge] Fix: avoid nil-pointer panic when atmosConfig is nil in Merge --------- Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
what
describe stacksflag--component-typesfrom aStringto aStringSlice:cmd/describe_stacks.go:String("component-types", "", ...)→StringSlice("component-types", nil, ...)cmd/describe_stacks_test.go:TestSetCliArgs_ComponentTypes_StringSliceasserts--component-types=terraform,helmfile→[]string{"terraform","helmfile"}why
trying to get stringSlice value of flag of type stringwhen--component-typeswas registered as a String, the command later read it as a []string, triggering the pflag runtime error.[]stringinDescribeStacksArgs), eliminating the error for:atmos describe stacks --format=json --component-types=terraform
--component-types=terraform,helmfile--component-types=terraform --component-types=helmfile--sections).Backwards compatibility: single-value usage (
--component-types=terraform) continues to work. Scripts that previously passed comma-separated values still work because pflag’sStringSliceaccepts CSV.Atmos Breaking Change:
Commit: 8957ff6
Date: 2025-06-05T10:08:15-07:00
First release tag containing the commit: v1.179.0
Present in all subsequent releases checked, including v1.188.0.
references
atmos describe stacks --format=json --component-types=terraformon v1.188.0 (darwin/arm64) → pflag type error.StringSliceto match[]stringin args struct.args.ComponentTypesas expected.Summary by CodeRabbit
New Features
Bug Fixes
Tests