fix: dont clobber static config with overrides #50
Conversation
WalkthroughThe pull request updates the Terraform configuration for managing stack settings. It refines the merging logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant M as Module "deep"
participant C as Common Config
participant S as Stack Config
participant R as Runtime Overrides
Note over M: Merge order: Common -> Stack -> Runtime
M->>C: Retrieve common configuration
M->>S: Retrieve stack-specific configuration
M->>R: Retrieve runtime overrides
M->>M: Merge configurations (C, then S, then R)
M-->>System: Apply final configuration
Possibly related PRs
Suggested reviewers
Poem
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 (1)
main.tf (1)
310-322: Deep Merge Logic Update:
The updated deep merge configuration in themodule "deep"block now combines three layers of configuration in the proper order: the common configuration, the static StackConfig values, and finally the runtime overrides. The inline comments clearly explain the intended override precedence. One suggestion is to consider validating that the output of thejsondecodecall is indeed a map (or the expected type) before merging, which could help guard against malformed runtime override inputs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
main.tf(1 hunks)tests/main.tftest.hcl(2 hunks)
🧰 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 (2)
tests/main.tftest.hcl (2)
220-264: Full Runtime Overrides Test:
This test block defines a comprehensive set of runtime override settings that update almost every field in the stack configuration (fromadministrativetodrift_detection_timezone). The detailed override values paired with targeted assertions ensure that the deep merge functionality is correctly applying runtime overrides on top of the static configuration.
459-664: Partial Runtime Overrides Test:
This test verifies that providing only a subset of runtime override settings (in this case, only overridingadministrative) leaves the other static configuration values intact. The assertions confirm that the deep merge does not inadvertently clobber unspecified settings. This provides good coverage against overzealous merging.
🤖 I have created a release *beep* *boop* --- ## [1.0.0](v0.7.0...v1.0.0) (2025-02-04) ### ⚠ BREAKING CHANGES * TF Version pinning has been updated to require terraform v1.9+ and OpenTofu v1.7+. Please update the consuming root module accordingly. (#43) * Scopes `tfvars` + `default_tf_workspace_enabled` settings under `automation_settings` object in StackConfig schema. [See README](https://github.com/masterpointio/terraform-spacelift-automation?tab=readme-ov-file#what-goes-in-a-stack-config-file-eg-stacksdevyaml-stackscommonyaml-stackyaml-and-similar) for example of StackConfig schema. (#42) ### Features * adds support for description as a templatestring ([#43](#43)) ([1bbb74f](1bbb74f)) * adds the runtime_overrides variable + tests ([#44](#44)) ([6030f94](6030f94)) * feat: allow specifying `space_name` that maps to space_id #46 * **schema:** adds initial JSON schema + StackConfig changes ([#42](#42)) ([f247b5e](f247b5e)) ### Bug Fixes * cron validator regex escape characters ([#45](#45)) ([81a386b](81a386b)) * dont clobber static config with overrides ([#50](#50)) ([b352674](b352674)) * external space changed so update test data ([#51](#51)) ([569d8d4](569d8d4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Gowie <[email protected]>
what
runtime_overridesfor a given stack to be included in the deep merge instead of TF's simple merge.why
runtime_overrideswith only one value in the overrides, it would overwrite the static StackConfigstack_settingsfor that stack and result in only including the runtime_overrides + the variable defaults. Not good 💥labelsand he was checking a common label... sadly that didn't catch this bug even though he was spot on that there was bug here!!! Luckily, integration testing surfaced this and then unit testing enabled me to quickly reproduce in the module. Now we have a solid fix 🎉references
Summary by CodeRabbit