Skip component if metadata.enabled is set to false
#756
Conversation
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the functionality of the Changes
Assessment against linked issues
Suggested labels
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
|
pkbhowmick
changed the title
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
pkbhowmick
changed the title
|
@coderabbitai review |
![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
🧹 Outside diff range and nitpick comments (2)
internal/exec/stack_utils.go (1)
Line range hint
187-196: Clarify relationship between metadata.enabled and vars.enabledThere's an existing
IsComponentEnabledfunction that checksenabledin the vars section, while we're now introducingenabledin the metadata section. This could lead to confusion about which flag takes precedence.Consider:
- Documenting the relationship between these two flags
- Potentially deprecating one in favor of the other
- Implementing clear precedence rules if both are needed
pkg/spacelift/spacelift_stack_processor.go (1)
177-177: Consider adding a clarifying comment for the skip condition.While the logic is sound, adding a comment would help future maintainers understand the two skip conditions:
- Abstract components (existing behavior)
- Disabled components (new behavior)
Consider adding this comment above the condition:
+ // Skip processing for abstract components and components with metadata.enabled=false if componentIsAbstract || !ComponentIsEnabled {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
internal/exec/stack_utils.go(3 hunks)internal/exec/utils.go(2 hunks)pkg/spacelift/spacelift_stack_processor.go(1 hunks)
🔇 Additional comments (6)
internal/exec/stack_utils.go (3)
57-61: Function signature update looks good, warrior! 💪
The updated function signature and documentation clearly communicate the new capability to determine component enablement status. This aligns perfectly with the PR's objective of supporting the metadata.enabled flag.
94-94: Return statement update is battle-ready! 🛡️
The return statement correctly includes the new componentIsEnabled flag, maintaining a clean and logical order of return values.
Line range hint 57-94: Verify usage of the new return value
Let's ensure all callers of ProcessComponentMetadata are updated to handle the new componentIsEnabled return value correctly.
✅ Verification successful
All callers properly handle the componentIsEnabled return value
The verification shows that all callers of ProcessComponentMetadata correctly handle the new return value:
spacelift_stack_processor.go: Skips processing when component is disabled withif componentIsAbstract || !ComponentIsEnabledinternal/exec/utils.go: Has two usages:- First usage properly checks and returns early with
if !componentIsEnabled - Second usage intentionally ignores the enabled status with
_, _placeholder variables, which is valid since it only needs the base component name
- First usage properly checks and returns early with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to ProcessComponentMetadata
echo "Searching for ProcessComponentMetadata calls..."
rg "ProcessComponentMetadata\(" --type go -A 2
Length of output: 1159
pkg/spacelift/spacelift_stack_processor.go (1)
175-177: LGTM! Clean implementation of the component skip logic.
The changes effectively implement the metadata.enabled flag functionality by:
- Capturing the enabled status from
ProcessComponentMetadata - Skipping disabled components early in the processing flow
This aligns perfectly with the PR objective of preventing workspace creation for disabled components.
Let's verify the integration with the metadata processing:
✅ Verification successful
Verified: The enabled flag implementation is robust and correctly integrated
The ProcessComponentMetadata function implementation shows:
- Default enabled state is
true(line 64) - Proper type assertion and existence check for the
enabledflag (line 79) - Correct boolean logic to set
componentIsEnabled = falseonly when explicitly disabled (lines 79-80)
This confirms that the skip logic in the reviewed code is properly supported by the underlying implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ProcessComponentMetadata function implementation
# to ensure it correctly handles the metadata.enabled flag
# Search for the ProcessComponentMetadata function definition
ast-grep --pattern 'func ProcessComponentMetadata($_, $_) ($_, $_, $_, $_) {
$$$
}'
Length of output: 2935
internal/exec/utils.go (2)
132-135: Clean implementation of component skip functionality!
The early return pattern when componentIsEnabled is false efficiently implements the requirement to skip components based on the metadata.enabled flag. This aligns perfectly with the PR objectives.
579-579: Verify if componentIsEnabled check is needed here.
While discarding unused return values is a common Go pattern, let's verify that skipping the component isn't required at this point in the code flow.
✅ Verification successful
Discarding componentIsEnabled is correct in this context
The code is handling this properly. We can see two distinct usages of ProcessComponentMetadata:
- First usage checks
componentIsEnabledand returns early if disabled - Second usage (our line in question) is in a different context where component skipping was already handled upstream, focusing only on getting the base component name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any other code paths handle component skipping for this function
# Search for other uses of ProcessComponentMetadata in the codebase
rg -A 5 "ProcessComponentMetadata.*componentIsEnabled"
# Search for any component skip logic in ProcessStacks
ast-grep --pattern 'func ProcessStacks($$$) {
$$$
if $_ {
return $_
}
$$$
}'
Length of output: 147
Script:
#!/bin/bash
# Let's try a different approach to find the usage context
# Find the ProcessStacks function implementation
ast-grep --pattern 'func ProcessStacks($$$) {
$$$
}'
# Search for any component skip checks in the same file
rg -B 2 -A 5 "componentIsEnabled.*skip" internal/exec/utils.go
# Look for the full ProcessComponentMetadata usage
rg -B 2 -A 2 "ProcessComponentMetadata" internal/exec/utils.go
Length of output: 699
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
pkbhowmick
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
internal/exec/helmfile.go (1)
67-67: Consider enhancing the log message with component details.The current log message could be more informative for debugging purposes.
- u.LogInfo(cliConfig, "component is disabled and skipped") + u.LogInfo(cliConfig, fmt.Sprintf("component '%s' is disabled and skipped", info.ComponentFromArg))internal/exec/terraform.go (1)
73-76: LGTM! Strategic placement of the component skip check.The placement of this check is optimal - early enough to prevent unnecessary operations but after essential context is loaded. The logging level and message are appropriate for operational visibility.
However, consider adding a debug log with the component name for better traceability:
if info.ComponentIsDisabled { - u.LogInfo(cliConfig, "component is disabled and skipped") + u.LogInfo(cliConfig, fmt.Sprintf("component '%s' is disabled and skipped", info.ComponentFromArg)) return nil }pkg/schema/schema.go (1)
199-199: Add serialization tags for consistency.The field addition aligns well with the PR objectives. However, for consistency with other fields in the struct, consider adding the standard serialization tags.
- ComponentIsDisabled bool + ComponentIsDisabled bool `yaml:"component_is_disabled" json:"component_is_disabled" mapstructure:"component_is_disabled"`internal/exec/utils.go (1)
579-581: Consider adding a line break for better readability.The functionality is correct, but the code readability could be improved by adding a line break between the ProcessComponentMetadata call and the struct field assignment.
_, baseComponentName, _, componentIsDisabled := ProcessComponentMetadata(configAndStacksInfo.ComponentFromArg, configAndStacksInfo.ComponentSection) configAndStacksInfo.BaseComponentPath = baseComponentName + configAndStacksInfo.ComponentIsDisabled = componentIsDisabled
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
internal/exec/helmfile.go(1 hunks)internal/exec/stack_utils.go(3 hunks)internal/exec/terraform.go(1 hunks)internal/exec/utils.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/spacelift/spacelift_stack_processor.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/stack_utils.go
🔇 Additional comments (4)
internal/exec/helmfile.go (1)
66-70: LGTM! Strategic placement of the component skip check.
The implementation is clean and well-placed - it occurs after stack processing but before any resource-intensive operations like helmfile execution or kubeconfig downloads. This ensures we fail fast and avoid unnecessary work.
Let's verify the component skip behavior:
✅ Verification successful
Component skip check is consistently implemented across execution paths
The verification confirms that the component skip check is properly implemented:
- Consistent early exit pattern in both
helmfile.goandterraform.go - Flag is correctly defined in the schema (
pkg/schema/schema.go) - Flag is properly set in
utils.gobefore execution
The placement and implementation are battle-ready across all execution paths. This is a solid defense against unnecessary resource operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the component skip check is consistently implemented across related files
# Test: Check if other execution paths also handle the disabled components
rg -A 5 "ComponentIsDisabled"
# Test: Verify the schema definition of this flag
ast-grep --pattern 'type ConfigAndStacksInfo struct {
$$$
ComponentIsDisabled bool
$$$
}'
Length of output: 5437
pkg/spacelift/spacelift_stack_processor.go (1)
175-177: LGTM! Clean implementation of the component skip feature.
The changes effectively implement the metadata.enabled flag functionality by:
- Capturing the new
componentIsDisabledreturn value fromProcessComponentMetadata - Adding it to the early exit conditions alongside
componentIsAbstract
This implementation aligns perfectly with the PR objectives of skipping components when metadata.enabled is false.
Let's verify the integration with the metadata processing:
internal/exec/terraform.go (1)
73-76: Verify consistent behavior across terraform commands.
Let's ensure the disabled state is handled consistently across all terraform operations.
✅ Verification successful
Disabled component handling is consistently implemented across terraform operations
The verification shows that the disabled component check is properly integrated into the main terraform execution flow before any terraform commands (plan, apply, destroy, init) are executed. The check occurs at the component processing level through IsComponentEnabled function, ensuring uniform handling across all operations.
Key findings:
- The disabled state check is performed at the component metadata processing level via
ProcessComponentMetadata - All terraform operations flow through the same execution path where this check is applied
- The implementation follows a fail-fast pattern, preventing any terraform operations on disabled components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other locations where terraform commands are executed
# to verify consistent handling of disabled components
# Look for terraform command execution patterns
rg -A 5 "terraform.*(?:plan|apply|destroy|init)" --type go
# Look for component state checks
rg -A 3 "Component.*(?:disabled|enabled|state)" --type go
Length of output: 17586
internal/exec/utils.go (1)
132-135: LGTM! Clean implementation of the component skip feature.
The early return pattern when a component is disabled is well implemented. Returning nil error is appropriate as skipping a disabled component is an expected behavior, not an error condition.
|
The flag should be metadata.enabled, not disabled. In general, we prefer feature flags to be the affirmative so we avoid double negatives. See task. |
|
Please see coderabbit for more descriptive error logs |
Yes, sure. In that case, are all components by default disabled? I mean do users have to always manually set this enabled field? |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
By default, components are enabled unless we set enabled false. |
Okay, going to implement it. Just one heads up, here is one downside using enabled variable name where the default value is true, in this case we need to manually set true everywhere as boolean default value is false |
pkbhowmick
changed the title
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
website/docs/core-concepts/stacks/define-components.mdx (2)
218-219: Consider rephrasing for active voice.The explanation is technically accurate but could be more direct. Consider this revision:
-The `metadata.enabled` parameter controls whether a component is included in deployment. By default, components are enabled. Setting `metadata.enabled` to `false` skips the component entirely—no workspace is created, and no Terraform commands are executed. Disabling a component does not cause deletion. It just signals that it's no longer managed by Atmos. +The `metadata.enabled` parameter determines if Atmos should include a component in deployment. Components are enabled by default. When you set `metadata.enabled` to `false`, Atmos completely skips the component—it won't create a workspace or execute any Terraform commands. When you disable a component, Atmos won't delete it but simply stops managing it.🧰 Tools
🪛 LanguageTool
[style] ~218-~218: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ...e is created, and no Terraform commands are executed. Disabling a component does not cause d...(REP_PASSIVE_VOICE)
226-235: Add more context to the example.The example is clear but could benefit from additional context about common use cases.
Consider adding a comment explaining when you might want to disable a component, such as:
# Disable a component in a specific environment +# Example: Disable VPC creation in development environments +# where you want to use an existing VPC components: terraform: vpc:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/define-components.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/define-components.mdx
[style] ~218-~218: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ...e is created, and no Terraform commands are executed. Disabling a component does not cause d...
(REP_PASSIVE_VOICE)
🔇 Additional comments (1)
website/docs/core-concepts/stacks/define-components.mdx (1)
236-236: LGTM! Clear and concise conclusion.
The closing statement effectively summarizes the benefit of using the metadata.enabled flag.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@pkbhowmick we need to import |
|
Oh, I forgot, we have the Just change this to an |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Done, CI passed now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
website/docs/core-concepts/stacks/define-components.mdx (2)
218-219: Consider using more active voice in the description.The description is accurate but could be more engaging with active voice.
-The `metadata.enabled` parameter controls whether a component is included in deployment. By default, components are enabled. Setting `metadata.enabled` to `false` skips the component entirely—no workspace is created, and no Terraform commands are executed. Disabling a component does not cause deletion. It just signals that it's no longer managed by Atmos. +The `metadata.enabled` parameter determines component inclusion in deployments. By default, Atmos enables all components. When you set `metadata.enabled` to `false`, Atmos skips the component entirely—it won't create workspaces or execute Terraform commands. Disabling a component tells Atmos to stop managing it, without causing deletion.🧰 Tools
🪛 LanguageTool
[style] ~218-~218: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ...e is created, and no Terraform commands are executed. Disabling a component does not cause d...(REP_PASSIVE_VOICE)
224-236: Consider enhancing the example with additional context.While the example is clear, it could be more educational by showing both enabled and disabled components in the same stack.
# Disable a component in a specific environment components: terraform: vpc: metadata: type: real enabled: false vars: name: primary-vpc + # This component will be deployed normally + eks: + metadata: + type: real + # enabled: true is the default + vars: + name: primary-cluster
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/define-components.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/define-components.mdx
[style] ~218-~218: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ...e is created, and no Terraform commands are executed. Disabling a component does not cause d...
(REP_PASSIVE_VOICE)
🔇 Additional comments (2)
website/docs/core-concepts/stacks/define-components.mdx (2)
216-217: LGTM! Clear and descriptive section header.
The section header effectively introduces the new feature.
220-222: LGTM! Clear distinction between vars.enabled and metadata.enabled.
The note effectively clarifies the difference between component-level feature flags and Atmos's component management.
aknysh
changed the title
metadata.enabled is set to false
|
These changes were released in v1.101.0. |
what
why
metadata.enabledtofalsein the stack manifest w/o affecting/changing/disabling the Terraform componentsworking demo
references
DEV-2150
Summary by CodeRabbit
Summary by CodeRabbit
New Features
metadata.enabledparameter to conditionally include components in deployment.Bug Fixes