fix: common labels merge with stack labels #9
Conversation
WalkthroughThe pull request introduces several enhancements to the configuration files related to Spacelift automation. A new package, Changes
Possibly related PRs
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)
examples/complete/components/spacelift-automation/stacks/common.yaml (1)
4-5: Consider using more descriptive label namesWhile adding labels to common.yaml is correct, consider using more descriptive names than
common_labelto better indicate the purpose or category of the label (e.g.,environment:shared,tier:common, etc.).main.tf (2)
229-231: LGTM! Consider documenting the behavior in variablesThe implementation correctly enables list appending for labels using Cloud Posse's deep merge module. This fixes the issue where common labels were being overwritten by stack-specific labels.
Consider adding a description of this behavior in the module's variables documentation:
variable "append_list_enabled" { type = bool default = true - description = "Enable list appending in deep merge" + description = "Enable list appending in deep merge. When true, lists (like labels) from common.yaml will be appended to stack-specific lists instead of being overwritten." }
Line range hint
182-195: Consider adding label format validationThe label merging logic is comprehensive, handling administrative, dependency, and folder labels correctly. To prevent issues, consider adding validation for custom labels format.
Add a validation check in the Spacelift stack resource:
resource "spacelift_stack" "default" { # ... existing configuration ... + lifecycle { + precondition { + condition = alltrue([for label in local.labels[each.key] : can(regex("^[a-z0-9][a-z0-9-_]*[a-z0-9]$", label))]) + error_message = "Labels must consist of lowercase alphanumeric characters, hyphens, or underscores, and must start and end with an alphanumeric character" + } + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
aqua.yaml(1 hunks)examples/complete/components/spacelift-automation/stacks/common.yaml(1 hunks)examples/complete/components/spacelift-automation/stacks/example.yaml(1 hunks)main.tf(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/complete/components/spacelift-automation/stacks/example.yaml
🧰 Additional context used
📓 Path-based instructions (1)
main.tf (1)
Pattern **/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
🔇 Additional comments (1)
aqua.yaml (1)
16-16: LGTM! Consider checking for newer versions
The addition of spacectl is appropriate for Spacelift automation. The version is correctly pinned for reproducibility.
✅ Verification successful
Version v1.8.0 is up to date
The currently used version of spacectl (v1.8.0) matches the latest release, so no update is needed at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for latest spacectl versions
gh api repos/spacelift-io/spacectl/releases/latest --jq .tag_name
Length of output: 74
| @@ -226,6 +226,9 @@ module "deep" { | |||
| for_each = local._root_module_stack_configs | |||
| # Stack configuration will take precedence and overwrite the conflicting value from the common configuration (if any) | |||
| maps = [local._common_configs[each.value.root_module], each.value] | |||
|
|
|||
| # To support merging labels from common.yaml, we need lists to append instead of overwrite | |||
| append_list_enabled = true | |||
what
append_list_enabledand set totruefor our usage of Cloud Posse's deep merge module.why
stacks/common.yamlare being overwritten by the stack specific labels in theirstacks/<stack_file_name>.yaml, which is not desired. The labels should be concatenated together, not overwrite.references