Skipping disabled components on atmos describe afffected #942
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new utility function Changes
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/describe/describe_affected_test.go (2)
104-106: Uset.Logfinstead oft.Log(fmt.Sprintf()).Replace the verbose logging with the more idiomatic Go testing approach.
-affectedYaml, err := u.ConvertToYAML(affected) -assert.Nil(t, err) -t.Log(fmt.Sprintf("\nAffected components and stacks:\n%v", affectedYaml)) +affectedYaml, err := u.ConvertToYAML(affected) +assert.Nil(t, err) +t.Logf("\nAffected components and stacks:\n%v", affectedYaml)🧰 Tools
🪛 golangci-lint (1.62.2)
106-106: S1038: should use t.Logf(...) instead of t.Log(fmt.Sprintf(...))
(gosimple)
109-111: Enhance test coverage for component filtering.The test only verifies that "disabled-component" is not in the affected list. Consider adding test cases for:
- Components that are explicitly enabled
- Components without the enabled flag
- Multiple disabled components
-for _, a := range affected { - assert.NotEqual(t, "disabled-component", a.Component, "Disabled component should not be included in affected list") -} +// Verify disabled components are filtered out +for _, a := range affected { + assert.NotEqual(t, "disabled-component", a.Component, "Disabled component should not be included in affected list") + assert.NotEqual(t, "another-disabled-component", a.Component, "Another disabled component should not be included in affected list") +} + +// Verify enabled components are included +foundEnabledComponent := false +for _, a := range affected { + if a.Component == "enabled-component" { + foundEnabledComponent = true + break + } +} +assert.True(t, foundEnabledComponent, "Enabled component should be included in affected list")internal/exec/describe_affected_utils.go (1)
556-561: Consider explicit handling of missingenabledfield.Currently, components without an
enabledfield in their metadata are processed. Consider making this behavior explicit by documenting it or setting a default value.// Skip disabled components if enabled, ok := metadataSection["enabled"].(bool); ok { if !enabled { continue } +} else { + // If enabled field is not present, component is considered enabled by default + u.LogTrace(atmosConfig, fmt.Sprintf("Component %s has no enabled field, treating as enabled by default", componentName)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exec/describe_affected_utils.go(2 hunks)pkg/describe/describe_affected_test.go(1 hunks)tests/fixtures/scenarios/complete/stacks/orgs/cp/tenant1/ue2/test-1.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/fixtures/scenarios/complete/stacks/orgs/cp/tenant1/ue2/test-1.yaml
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/describe/describe_affected_test.go
106-106: S1038: should use t.Logf(...) instead of t.Log(fmt.Sprintf(...))
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
osterman
added
patch
|
@shirkevich can you please review and address the comments |
Done, don't know how to create proper fixture and test to validate this on CI. Tested it locally with my env |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/describe_affected_utils.go (1)
1524-1533: Enhance log message format for consistency.The implementation follows the standardization guidelines well. Consider updating the log message format to match the exact format from the learnings.
- u.LogTrace(atmosConfig, fmt.Sprintf("Skipping disabled component %s", componentName)) + u.LogTrace(atmosConfig, fmt.Sprintf("Skipping disabled component '%s'", componentName))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/describe_affected_utils.go(3 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#942
File: internal/exec/describe_affected_utils.go:802-807
Timestamp: 2025-01-16T11:41:35.531Z
Learning: When checking if a component is enabled in Atmos, use standardized helper function that includes logging. The function should check the `enabled` field in the component's metadata section and log a trace message when skipping disabled components.
internal/exec/describe_affected_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#942
File: internal/exec/describe_affected_utils.go:802-807
Timestamp: 2025-01-16T11:41:35.531Z
Learning: When checking if a component is enabled in Atmos, use standardized helper function that includes logging. The function should check the `enabled` field in the component's metadata section and log a trace message when skipping disabled components.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/describe_affected_utils.go (2)
556-559: LGTM! Clean refactoring to use the helper function.The extraction of the component enabled check into a helper function improves code maintainability and follows DRY principles.
800-803: LGTM! Consistent implementation across sections.The helper function is consistently used in both Terraform and Helmfile sections, effectively reducing code duplication.
|
These changes were released in v1.153.1. |
what
atmos describe affectedcommandwhy
metadata.enabled: falseshould be excluded from the affected components listdescribe affectedcommand by only showing components that would actually be processedSummary by CodeRabbit
New Features
Bug Fixes