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: use hcl.MergeBodies instead of configs.MergeBodies for pro… #29000

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

mildwonkey
Copy link
Contributor

…vider configuration

Previously, Terraform would return an error if the user supplied provider configuration via interactive input iff the configuration provided on the command line was missing any required attributes - even if those attributes were already set in config.

That error came from configs.MergeBody, which was designed for overriding valid configuration. It expects that the first ("base") body has all required attributes. However in the case of interactive input for provider configuration, it is perfectly valid if either or both bodies are missing required attributes, as long as the final body has all required attributes. hcl.MergeBodies works very similarly to configs.MergeBodies, with a key difference being that it only checks that all required attributes are present after the two bodies are merged.

I've updated the existing test to use interactive input vars and a schema with all required attributes. This test failed before switching from configs.MergeBodies to hcl.MergeBodies.

Fixes #28956

…vider configuration

Previously, Terraform would return an error if the user supplied provider configuration via interactive input iff the configuration provided on the command line was missing any required attributes - even if those attributes were already set in config.

That error came from configs.MergeBody, which was designed for overriding valid configuration. It expects that the first ("base") body has all required attributes. However in the case of interactive input for provider configuration, it is perfectly valid if either or both bodies are missing required attributes, as long as the final body has all required attributes. hcl.MergeBodies works very similarly to configs.MergeBodies, with a key difference being that it only checks that all required attributes are present after the two bodies are merged.

I've updated the existing test to use interactive input vars and a schema with all required attributes. This test failed before switching from configs.MergeBodies to hcl.MergeBodies.
@mildwonkey mildwonkey requested a review from a team June 21, 2021 20:08
@mildwonkey
Copy link
Contributor Author

On second thought, hcl.MergeBodies doesn't handle dynamic blocks - this will take some more thought.

@mildwonkey mildwonkey closed this Jun 22, 2021
@mildwonkey mildwonkey reopened this Jun 22, 2021
@mildwonkey
Copy link
Contributor Author

Reopening! Any dynamic blocks have already been expanded by the time we get to buildProviderConfig. I've added a test in the command package that shows that terraform can handle the combination of provider input vars + dynamic provider config (please don't) + required attributes. This test failed when buildProviderConfig still used configs.MergeBodies.

…s with dynamic configuration + required + interactive input merging

This test failed when buildProviderConfig still used configs.MergeBodies instead of hcl.MergeBodies
@mildwonkey mildwonkey force-pushed the mildwonkey/b-interactive-input-vars branch from 64195ab to 50cb740 Compare June 22, 2021 14:48
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks plausible to me!

Since the bug this is addressing is such an edge case and has been encountered only once over several years, I'd vote for letting this one soak in the 1.1 branch for a bit (and asking the reporter to explicitly test from there) rather than immediately backporting this to 1.0, even though it's a bug fix. I trust the code you've written here, but hcl.MergeBodies is some old code that we've not made extensive use of so far and so while I don't have any specific knowledge of any misbehavior in its implementation, hopefully it living through the 1.1 alpha/beta releases will give an opportunity for a variety of people to exercise it and see if they encounter anything odd that we might not have considered here.

@mildwonkey mildwonkey merged commit 0960106 into main Jun 25, 2021
@mildwonkey mildwonkey deleted the mildwonkey/b-interactive-input-vars branch June 25, 2021 12:51
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive prompts ignore already specified inputs
2 participants