feat: Support Component Lock with metadata.locked
#908
Conversation
milldr
added
minor
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive mechanism to lock components in Atmos, preventing modifications while maintaining read access. By adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
🧹 Nitpick comments (2)
internal/exec/utils.go (2)
135-135: Maintain consistent ordering.It might be clearer to set
ComponentIsLockedimmediately after settingComponentIsEnabledto group similar fields and make the code more readable.133 componentMetadata, baseComponentName, componentIsAbstract, componentIsEnabled, componentIsLocked := ProcessComponentMetadata(component, componentSection) 134 configAndStacksInfo.ComponentIsEnabled = componentIsEnabled -135 configAndStacksInfo.ComponentIsLocked = componentIsLocked +135 configAndStacksInfo.ComponentIsLocked = componentIsLocked
606-606: Similar comment on consistent ordering.For consistency, consider placing
ComponentIsLockedright after settingComponentIsEnabled.603 _, baseComponentName, _, componentIsEnabled, componentIsLocked := ProcessComponentMetadata(configAndStacksInfo.ComponentFromArg, configAndStacksInfo.ComponentSection) 604 configAndStacksInfo.BaseComponentPath = baseComponentName 605 configAndStacksInfo.ComponentIsEnabled = componentIsEnabled -606 configAndStacksInfo.ComponentIsLocked = componentIsLocked +607 configAndStacksInfo.ComponentIsLocked = componentIsLocked
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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)website/docs/core-concepts/stacks/define-components.mdx(1 hunks)
🔇 Additional comments (12)
internal/exec/stack_utils.go (5)
58-58: Nicely expanded comment.Clear documentation improves maintainability. Good job clarifying that we also check the locked state here.
62-62: Expanded return signature.Returning the locked state is consistent with the rest of the code. This ensures that callers can decide how to handle a locked component.
66-66: Good default initialization.Setting
componentIsLockedto false by default is intuitive. Components remain unlocked unless explicitly stated otherwise.
86-90: Conditional assignment of locked state.The logic for reading
lockedfrom metadata is straightforward. This effectively flags components as locked in subsequent checks.
103-103: Returning locked status.Including
componentIsLockedin the returned tuple finalizes the integration. This is well-aligned with the updated usage across the codebase.internal/exec/helmfile.go (1)
97-105: Locked component guard.Preventing modification commands for locked components is consistent with the PR’s objective. This ensures safer state management.
pkg/spacelift/spacelift_stack_processor.go (1)
175-175: Ignored locked state.Using the underscore indicates ignoring the locked flag here. If needed later, it can be utilized without disrupting existing code.
internal/exec/terraform.go (1)
125-134: Locked component enforcement.Smart to prevent destructive commands on locked components. This matches the approach in the Helmfile logic.
pkg/schema/schema.go (1)
234-234: Great introduction of the locked field.Adding
ComponentIsLockedprovides a clear way to track and enforce locking at the schema level.internal/exec/utils.go (2)
133-133: Good job capturing the locked status.Storing
componentIsLockedimmediately after retrieving it fromProcessComponentMetadataensures the correct flow of information.
603-603: Good reuse ofProcessComponentMetadata.This ensures that the locked status is always determined at the same time as abstract/enabled status.
website/docs/core-concepts/stacks/define-components.mdx (1)
237-259: Helpful new section.Documenting
metadata.lockedclarifies how teams can control authorized usage. The example is concise and demonstrates a real-life scenario for safeguarding critical infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
examples/demo-metadata/atmos.yaml (1)
10-10: Clean up the battlefield - remove trailing spaces.There are trailing spaces at the end of line 10.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
examples/demo-metadata/components/terraform/myapp/main.tf (1)
11-16: STRENGTHEN YOUR DEFENSES, BRAVE ONE!While the HTTP data source is functional, we can make it more resilient in battle:
data "http" "weather" { url = local.url request_headers = { User-Agent = "curl" } + retry { + attempts = 3 + min_delay_ms = 1000 + max_delay_ms = 5000 + } + timeout = "10" }examples/demo-metadata/components/terraform/myapp/outputs.tf (1)
1-27: WELL-STRUCTURED OUTPUTS, BUT LET'S POLISH OUR PROCLAMATIONS!Your output structure is strong, but let's refine the descriptions for clarity:
output "location" { value = var.location - description = "Location of the weather report." + description = "Location for the weather report" } output "lang" { value = var.lang - description = "Language which the weather is displayed." + description = "Language in which the weather is displayed" } output "units" { value = var.units - description = "Units the weather is displayed." + description = "Units in which the weather is displayed" }examples/demo-metadata/components/terraform/myapp/variables.tf (1)
1-34: STRONG VARIABLE DECLARATIONS, FELLOW WARRIOR!Your variable structure is robust, but let's enhance the clarity of our battle plan:
- Improve descriptions:
variable "location" { - description = "Location for which the weather." + description = "Location for which to fetch the weather report" }
- Consider adding validation rules for critical variables:
variable "stage" { description = "Stage where it will be deployed" type = string + validation { + condition = contains(["dev", "staging", "prod"], var.stage) + error_message = "Stage must be one of: dev, staging, prod." + } } variable "units" { description = "Units in which the weather is displayed." type = string default = "m" + validation { + condition = contains(["m", "u", "M"], var.units) + error_message = "Units must be one of: m (metric), u (USCS), M (metric with wind in m/s)." + } }tests/test_cases.yaml (1)
219-233: Excellent negative test case for locked component protection!The test effectively validates that locked components cannot be modified, with a clear error message. Consider adding additional test cases for:
- Read-only operations (like plan) on locked components
- Other commands that should be blocked on locked components
examples/demo-metadata/components/terraform/myapp/README.md (1)
1-14: Add component locking information to features sectionSince this example is in the
demo-metadatadirectory and is used in the locking test cases, consider adding information about the component locking feature to demonstrate real-world usage ofmetadata.locked.🧰 Tools
🪛 LanguageTool
[typographical] ~3-~3: Consider using typographic quotation marks here.
Context: ...aform Weather Component This Terraform "root" module fetches weather information for ...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/demo-metadata/.gitignore(1 hunks)examples/demo-metadata/atmos.yaml(1 hunks)examples/demo-metadata/components/terraform/myapp/README.md(1 hunks)examples/demo-metadata/components/terraform/myapp/main.tf(1 hunks)examples/demo-metadata/components/terraform/myapp/outputs.tf(1 hunks)examples/demo-metadata/components/terraform/myapp/variables.tf(1 hunks)examples/demo-metadata/components/terraform/myapp/versions.tf(1 hunks)examples/demo-metadata/stacks/catalog/myapp.yaml(1 hunks)examples/demo-metadata/stacks/deploy/nonprod.yaml(1 hunks)examples/demo-metadata/stacks/deploy/prod.yaml(1 hunks)tests/test_cases.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- examples/demo-metadata/components/terraform/myapp/versions.tf
- examples/demo-metadata/stacks/catalog/myapp.yaml
- examples/demo-metadata/.gitignore
🧰 Additional context used
🪛 yamllint (1.35.1)
examples/demo-metadata/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 LanguageTool
examples/demo-metadata/components/terraform/myapp/README.md
[typographical] ~3-~3: Consider using typographic quotation marks here.
Context: ...aform Weather Component This Terraform "root" module fetches weather information for ...
(EN_QUOTES)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ... where it will be deployed. - location: Location for which the weather is repor...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~34-~34: Consider using typographic quotation marks here.
Context: ...ich the weather is reported. Default is "Los Angeles". - options: Options to customize the ...
(EN_QUOTES)
[uncategorized] ~35-~35: Loose punctuation mark.
Context: ...d. Default is "Los Angeles". - options: Options to customize the output. Defaul...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~35-~35: Consider using a typographic opening quote here.
Context: ...ons to customize the output. Default is "0T". - format: Specifies the output fo...
(EN_QUOTES)
[typographical] ~35-~35: Consider using a typographic close quote here.
Context: ... to customize the output. Default is "0T". - format: Specifies the output forma...
(EN_QUOTES)
[uncategorized] ~36-~36: Loose punctuation mark.
Context: ... the output. Default is "0T". - format: Specifies the output format. Default is...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~36-~36: Consider using a typographic opening quote here.
Context: ...Specifies the output format. Default is "v2". - lang: Language in which the wea...
(EN_QUOTES)
[typographical] ~36-~36: Consider using a typographic close quote here.
Context: ...cifies the output format. Default is "v2". - lang: Language in which the weathe...
(EN_QUOTES)
[uncategorized] ~37-~37: Loose punctuation mark.
Context: ...output format. Default is "v2". - lang: Language in which the weather will be d...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~37-~37: Consider using typographic quotation marks here.
Context: ...e weather will be displayed. Default is "en". - units: Units in which the weather ...
(EN_QUOTES)
[uncategorized] ~38-~38: Loose punctuation mark.
Context: ...be displayed. Default is "en". - units: Units in which the weather will be disp...
(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~38-~38: Consider using typographic quotation marks here.
Context: ...e weather will be displayed. Default is "m". ### Outputs - weather: The fetched ...
(EN_QUOTES)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...ther: The fetched weather data. - url: Requested URL. - stage`: Stage of depl...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ... data. - url: Requested URL. - stage: Stage of deployment. - location: Loca...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...tage: Stage of deployment. - location: Location of the reported weather. - la...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...cation of the reported weather. - lang: Language used for weather data. - `unit...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...anguage used for weather data. - units: Units of measurement for the weather da...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (6)
examples/demo-metadata/stacks/deploy/nonprod.yaml (1)
1-14: Well-structured nonprod configuration, warrior!The configuration follows best practices with schema validation and clear environment-specific settings. The absence of component locking is appropriate for a nonprod environment where changes are expected.
examples/demo-metadata/stacks/deploy/prod.yaml (1)
12-13: Strong protection for production components, commander!The
metadata.locked: truesetting effectively prevents accidental modifications to the production component while maintaining read access. This aligns perfectly with the PR's objective of enhancing deployment safety.examples/demo-metadata/atmos.yaml (2)
6-9: Battle-hardened terraform configuration, strategist!The configuration demonstrates strong security consciousness with:
apply_auto_approve: falserequiring explicit approvaldeploy_run_init: trueensuring proper initializationinit_run_reconfigure: truemaintaining configuration consistency
13-16: Well-defined stack boundaries, tactician!The stack configuration effectively manages deployment scope through:
- Explicit inclusion of deploy paths
- Strategic exclusion of defaults
examples/demo-metadata/components/terraform/myapp/main.tf (1)
1-9: STRONG WORK, WARRIOR! URL construction is well-fortified!The URL construction shows excellent defensive programming with proper URL encoding for all parameters. This is the way of the secure warrior!
Let's verify the URL construction with this battle plan:
✅ Verification successful
VICTORY! All variables are properly declared and documented, brave warrior!
The verification shows that all variables used in the URL construction (
location,options,format,lang, andunits) are properly defined in the correspondingvariables.tffile with clear descriptions. This ensures robust and maintainable code structure.The battle-tested URL construction can proceed with confidence!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all variables used in the URL construction are properly defined rg -A 1 'variable "(location|options|format|lang|units)"' .Length of output: 4016
tests/test_cases.yaml (1)
201-217: Strong test case for unlocked component deployment!The test case effectively validates that terraform deploy works as expected on unlocked components. The assertions are well-structured, checking both successful execution and clean stderr output.
Let's verify the test environment is properly set up:
✅ Verification successful
Test environment is properly configured for unlocked component validation!
The test case is well-structured and the environment is correctly set up:
- Component
myappis properly defined in the catalog with default configuration- The nonprod stack correctly imports and overrides the component settings
- No lock files are present, confirming the component is indeed unlocked
- The test's working directory and component name align with the actual configuration
This verification confirms that the test case will effectively validate terraform deployment behavior for unlocked components. Stand strong, warrior! Your test shall guard against unwanted regressions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the demo-metadata example directory contains necessary component configuration fd -t f "stack.y*ml" examples/demo-metadata/stacks/ -x cat {} \; | grep -A 5 "myapp:"Length of output: 88
Script:
#!/bin/bash # Let's check if the directory exists and list its contents ls -la examples/demo-metadata/stacks/ # Check for any yaml files in the examples/demo-metadata directory and its subdirectories fd -e yml -e yaml . examples/demo-metadata/ # Let's try to find any references to myapp in yaml files rg -l "myapp" examples/demo-metadata/Length of output: 669
Script:
#!/bin/bash # Let's check the content of the relevant files to verify the myapp component configuration cat examples/demo-metadata/stacks/catalog/myapp.yaml cat examples/demo-metadata/stacks/deploy/nonprod.yaml # Also check if there are any lock files that might affect the component fd -e lock.hcl . examples/demo-metadata/Length of output: 661
There was a problem hiding this comment.
Copying an entire example from something else to only add metadata as an example is too much technical debt to maintain, and doesn't focus the example enough on one small feature, which makes it difficult to understand what's necessary. In other words, all the other stuff like variables and outputs, etc. detract from the example.
This one feature should just be added to example/tests. Alternatively, a reduced example specifically focused on locking or unlocking could be created. But a generic example on metadata is far too broad.
There was a problem hiding this comment.
I agree, but I was thinking this would be a good place to start adding examples for metadata usage other than just metadata.locked, since there are many use cases.
Although, simple tests in example/tests would be fine by me too. Let me know which
There was a problem hiding this comment.
Tests failed anyway. Fixing those now. Done
There was a problem hiding this comment.
Alternatively, we also could have some example where we can define many advantaged usages, including metadata and relatives paths. I dont see any of the existing examples aligning well. We shouldn't over complicate examples/quick-start-advanced, and examples/tests intentionally contains invalid configuration.
Instead we could add 1 example, such as examples/test-advanced where it's a valid configuration with many advanced usage validation
There was a problem hiding this comment.
examples/tests intentionally contains invalid configuration.
The examples/tests directory is intentionally designed to contain invalid configurations. This is because tests are meant to validate both functional and non-functional scenarios, including edge cases and failure states. Adding configurations there—even if they are invalid—is exactly how we ensure robust testing of different scenarios.
In contrast, providing demos or examples serves a different purpose. Demos are optimized to showcase minimal, functional use cases for specific features, serving as a reference or starting point for users. Their goal is not to validate edge cases or invalid inputs.
The label test-advanced feels arbitrary to me, especially since there are already many advanced examples under examples/tests. The term "advanced" is highly subjective, and we aim for clarity and alignment with the intended use case. For instance, we already differentiate quick-start-simple and quick-start-advanced under the broader "quick-start" use case.
Also, I forgot to mention we need to update the JSON Schema.
There was a problem hiding this comment.
My confusion comes from trying to create tests with the new tests/test_cases.yaml functionality.
Those tests simply run an atmos command and then check for expected output, but in order to run any atmos command, atmos first must compile YAML for all stacks. Therefore examples/tests cannot be used for tests/test_cases.yaml since any command will always fail to compile intentionally.
Instead we can create or reuse a different example (what I was suggesting) or only include go tests (do not add tests/test_cases)
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
what
metadata.lockedwith componentswhy
metadata.lockedparameter prevents changes to a component while still allowing read operations. When a component is locked, operations that would modify infrastructure (liketerraform apply) are blocked, while read-only operations (liketerraform plan) remain available. By default, components are unlocked. Settingmetadata.lockedtotrueprevents any change operations.references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
metadata.lockedparameter.Bug Fixes
Tests