Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds the runtime_overrides variable + tests #44

Merged
merged 15 commits into from
Feb 3, 2025

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented Jan 23, 2025

what

  • Adds the runtime_overrides variable + tests to check usage
    • NOTE: The new overrides test is currently failing due to what I'm calling the "Optional Type Defaults Problem". I'm posting this to share the full use-case for what we're running into so I can see if others have an elegant way to handle this problem. Below is the output of the failing tests just so it's easy to find. Check out the OpenTofu Slack thread linked below for full details.
    • (Previously) Failing Test Output
  • Introduces StackConfig schema validation using the https://registry.terraform.io/providers/bpedman/jsonschema/latest/docs
  • Adds terraform-docs as a trunk action + config for that + removes pre-commit-config

why

  • runtime_overrides allows consumers of this child module to do logic within their root module to manage the final Stack config. This gives them additional flexibility beyond our static StackConfig configuration.

references

@Gowiem Gowiem self-assigned this Jan 23, 2025
@Gowiem Gowiem requested a review from a team as a code owner January 23, 2025 19:10
@Gowiem Gowiem force-pushed the feature/runtime-overrides branch from 4babc73 to 96d325e Compare January 28, 2025 06:19
Copy link
Member

@gberenice gberenice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The only thing that confuses me - this PR contains the changes introduced in the parallel PR #42. Is that intended?

@Gowiem
Copy link
Member Author

Gowiem commented Jan 29, 2025

@gberenice mind giving me another review? I added a new test to confirm the schema is working as expected 👍

gberenice
gberenice previously approved these changes Jan 29, 2025
Copy link
Member

@gberenice gberenice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I like it! Thanks, Matt 👏

@Gowiem Gowiem force-pushed the feature/runtime-overrides branch from 3aadae6 to f18df2b Compare January 30, 2025 18:55
@Gowiem Gowiem enabled auto-merge (squash) January 30, 2025 19:08
Copy link
Member

@westonplatter westonplatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will come back and look deeper after a client meeting

}

# Test the default-example stack with runtime overrides
run "test_default_example_stack_runtime_overrides" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After watching your rubber 🦆 loom and reading through the open tofu slack thread, I'm wondering if we have a test in this PR for this logic? I didn't see it after a first pass, but I may be overlooking test logic for that condition.

# variables.tf
variable "runtime_overrides" {
  type = object({
    administrative = optional(bool)
    branch = optional_rJemove(string)
  })

  default = {}
}

# terraform.tfvars
runtime_overrides = {
  administrative = false
}

# At runtime
var.runtime_overrides => { administrative = false }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonplatter This test that you commented on provides just that:

  1. We provide a value to runtime_overrides
  2. We confirm the values in the underlying resource match the overrides for the stack that we're trying to override.

Specifically for administrative: https://github.com/masterpointio/terraform-spacelift-automation/pull/44/files#diff-42b25f743b9dffd1816864939465621064627de79a1654c58e8f124ca64d66d9R267-R270

Were you looking for something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the first test confirms the administrative value, that looks good. I was thinking of a test for how branch is handled - namely, when does it show up in the run_overrides map vs when is it omitted because it is an optional override.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@westonplatter westonplatter Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That loom video helps a lot - I took from it that the type(any) means we are passing in anything, and the mega merge operation drives the final (defaults + any runtime overrides) outcome. There is no expected intermediate data structure with nulls.

The only remaining question I have (and this comes from my rails/web app developer days) is testing that an override doesn't suddenly alter or negatively impact the expected default values.

So in this example, ensure the default value for administrative comes through unaltered by the non-default space_id value.

# Test that the description is created correctly when non-default template string is used
run "test_description_is_created_correctly_when_non_default_template_string_is_used" {
  command = plan
  variables {
    description = "Space ID: $${stack_settings.space_id}"
  }

  assert {
    condition = spacelift_stack.default["root-module-a-test"].description == "Space ID: 123"
    error_message = "Description was not created correctly: ${jsonencode(local.configs)}"
  }

  # new test - ensure the default `administrative` value comes
  assert {
    condition = spacelift_stack.default["root-module-a-test"].administrative == true
    error_message = "Administrative was not created correctly: ${jsonencode(local.configs)}"
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, the suggested code/test above does not match what I was describing. Ok, after reading the code more, this is that I was trying to get at, #48.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes I suggested cause a failing. Looks like I don't understand enough of what's going on. Will take another look and follow up tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonplatter your PR doesn't look wrong and I get the test that you're trying to make now. See comment in your PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching the misnamed variable. I updated it and move the code around a bit, but it's ready to to add to this branch if you're open to testing for that logic situation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonplatter glad we got that in! Re-review when you can -- Thanks!

@Gowiem Gowiem force-pushed the feature/runtime-overrides branch from 5cae82e to 32b7454 Compare January 30, 2025 21:46
@westonplatter westonplatter mentioned this pull request Jan 31, 2025
@Gowiem Gowiem requested a review from westonplatter January 31, 2025 19:30
westonplatter
westonplatter previously approved these changes Feb 3, 2025
Copy link
Member

@westonplatter westonplatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look to me. The test coverage was really helpful in understanding the approach and implications of the changes.

Comment on lines +297 to +300
drift_detection_stacks = {
for stack, config in local.stack_configs :
stack => config if try(config.drift_detection_enabled, var.drift_detection_enabled)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice cleanup consolidation

Comment on lines +177 to +180
"space_name": {
"type": "string",
"description": "Spacelift space name, this will be translated to a space_id. Mutually exclusive with space_id"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty 🙏

@Gowiem Gowiem merged commit 6030f94 into main Feb 3, 2025
4 checks passed
@Gowiem Gowiem deleted the feature/runtime-overrides branch February 3, 2025 20:21
Gowiem added a commit that referenced this pull request Feb 4, 2025
## what

- Updates `runtime_overrides` for a given stack to be included in the
deep merge instead of TF's simple merge.


## why

- Short version: Properly merges runtime overrides on top of static
StackConfig values
- Long version: I found this bug when doing some diligent integration
testing in our mp-infra repo. Prior to this fix, if we included
`runtime_overrides` with only one value in the overrides, it would
overwrite the static StackConfig `stack_settings` for that stack and
result in only including the runtime_overrides + the variable defaults.
Not good 💥
- This is exactly what @westonplatter was working to avoid when he was
pushing me in this thread:
#44 (comment).
He added a test for this, but it was with `labels` and 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

- Follow up to #44 and #48


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
- Enhanced the configuration merging process to now apply runtime
overrides after base and stack-specific settings, offering greater
flexibility and dynamic control.
- **Tests**
- Expanded test coverage with additional validations and refined naming
to ensure consistent behavior and reliability of the configuration
adjustments.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Gowiem added a commit that referenced this pull request Feb 4, 2025
🤖 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants