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

terraform test: allow computed/mocked values override during planning #36227

Merged
merged 21 commits into from
Jan 8, 2025

Conversation

dsa0x
Copy link
Contributor

@dsa0x dsa0x commented Dec 17, 2024

Fixes #35851

This PR enables users to use mocked or overridden values when running a plan command. The attribute override_during can be set to plan or apply. This attribute can be set in the blocks for override_*, and mock_provider.

Target Release

1.11.0

Draft CHANGELOG entry

NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS

@dsa0x dsa0x force-pushed the sams/allow-mocked-values-planning branch from 6bfae8e to aea2a5a Compare December 17, 2024 17:16
@dsa0x dsa0x changed the title allow mocked values override during planning terraform test: allow mocked values override during planning Dec 18, 2024
@dsa0x dsa0x changed the title terraform test: allow mocked values override during planning terraform test: allow computed values override during planning Dec 18, 2024
@dsa0x dsa0x force-pushed the sams/allow-mocked-values-planning branch from 23ce4ef to 5031c9d Compare December 18, 2024 16:02
@dsa0x dsa0x force-pushed the sams/allow-mocked-values-planning branch 2 times, most recently from d724364 to 3cdb84a Compare December 19, 2024 10:09
@dsa0x dsa0x changed the title terraform test: allow computed values override during planning terraform test: allow computed/mocked values override during planning Dec 19, 2024
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

I think the implementation looks good, just not super happy with the chosen name. I'd be interested to know your thoughts!

internal/configs/mock_provider.go Outdated Show resolved Hide resolved
// When this attribute is set to true, the values specified in the override
// block will be used for computed attributes even when planning. Otherwise,
// the computed values will be set to unknown, just like in a real plan.
overrideComputed = "override_computed"
Copy link
Member

Choose a reason for hiding this comment

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

We have the command attribute available in the run blocks: https://developer.hashicorp.com/terraform/language/tests#run-blocks

I wonder if like compute_command that maps to a keyword of either plan or apply might be more clear about what is happening here? We can make it default to apply.

I think that makes it a bit more clear about what the attribute is actually doing. What do you think?

Copy link
Contributor Author

@dsa0x dsa0x Jan 7, 2025

Choose a reason for hiding this comment

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

Thanks for the suggestion. I contemplated quite a bit on the right name to communicate what's going on here, and you are right that override_computed seems a bit unclear. However, I have similar thoughts about compute_command, especially the compute part. That may be confusing. What about override_command/override_target/override_target_command = plan/apply? From a user POV, they have defined the override, and want it to be applied. That it is a computed attribute is more subtle.

Copy link
Member

Choose a reason for hiding this comment

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

I like override_command, I think that could work. Another idea I had was override_during - as an attribute it doesn't make much sense on it's own, but when coupled with apply or plan then it brings a lot of context eg. override_during = plan.

But, override_command is also good I think 👍

internal/moduletest/mocking/values.go Outdated Show resolved Hide resolved
@dsa0x dsa0x force-pushed the sams/allow-mocked-values-planning branch 2 times, most recently from 694d10b to dd3f791 Compare January 7, 2025 11:28
@dsa0x dsa0x force-pushed the sams/allow-mocked-values-planning branch 2 times, most recently from 2be1048 to 0cdeaa9 Compare January 7, 2025 11:44
@dsa0x dsa0x requested a review from liamcervante January 7, 2025 11:57
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

Sorry, I just noticed a couple of places where you have pointers and I don't think you need them. This doesn't affect the behaviour of anything though, but I think makes every thing look a bit neater.

Also, since we just merged the changes to the CHANGELOG process you should add a CHANGELOG entry in this PR now.

// of a real plan. This attribute indicates that the computed values
// should be overridden with the values specified in the override block,
// even when planning.
useForPlan *bool
Copy link
Member

Choose a reason for hiding this comment

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

sorry, I didn't notice this before - I don't think this needs to be a pointer. You can just make it a normal bool, and public so callers can read it directly.

We don't need to distinguish between "not set" and explicitly set to false right?

@@ -65,12 +72,39 @@ func decodeMockProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) {
return provider, diags
}

func extractOverrideDuring(content *hcl.BodyContent) (*string, hcl.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could also make this return a string without a pointer -> just return "apply" if the attribute isn't set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this section internal/configs/mock_provider.go#L317-L325, the field needs to be a pointer. This is necessary because, if the override_* block does not explicitly set a value for override_during, the global value should be applied. However, if the override_* block does set a value, it should take precedence over the global value. Using a pointer allows us to differentiate between an explicitly set value and a zero value.

Copy link
Member

Choose a reason for hiding this comment

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

ah nice! Makes sense!

liamcervante
liamcervante previously approved these changes Jan 8, 2025
@@ -65,12 +72,39 @@ func decodeMockProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) {
return provider, diags
}

func extractOverrideDuring(content *hcl.BodyContent) (*string, hcl.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

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

ah nice! Makes sense!

@dsa0x dsa0x marked this pull request as ready for review January 8, 2025 09:36
@dsa0x dsa0x requested a review from a team as a code owner January 8, 2025 09:36
@liamcervante
Copy link
Member

There's a lot of commits here, so I'd make sure to squash and merge this - you might do that be default already but just wanted to make sure.

@dsa0x dsa0x merged commit aec7c3c into main Jan 8, 2025
7 checks passed
@dsa0x dsa0x deleted the sams/allow-mocked-values-planning branch January 8, 2025 11:39
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.

Allow mocked values on resources during the planning stage in terraform test
2 participants