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: Fix required version constraint diags #25898

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

alisdair
Copy link
Contributor

If a module has multiple terraform.required_version constraints, any failures would point at the last constraint in the error diagnostics. If an earlier constraint was the actual problem, this leads to confusing errors like this:

    Error: Unsupported Terraform Core version

      on main.tf line 6, in terraform:
       6:   required_version = ">= 0.13.0"

    This configuration does not support Terraform version 0.13.0.

The error was due to storing the declaration range of the constraint as a pointer to the contents of a loop variable, which was later overwritten in later iterations of the loop. Instead we now use HCL's handy Range.Ptr method to create a direct pointer to the range struct.

Fixes #25833

If a module has multiple terraform.required_version constraints, any
failures would point at the last constraint in the error diagnostics. If
an earlier constraint was the actual problem, this leads to confusing
errors like this:

    Error: Unsupported Terraform Core version

      on main.tf line 6, in terraform:
       6:   required_version = ">= 0.13.0"

    This configuration does not support Terraform version 0.13.0.

The error was due to storing the declaration range of the constraint as
a pointer to the contents of a loop variable, which was later
overwritten in later iterations of the loop.  Instead we now use HCL's
handy Ptr() method to create a direct pointer to the range struct.
@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #25898 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/version_required.go 100.00% <100.00%> (ø)
states/statefile/version4.go 57.54% <0.00%> (-0.29%) ⬇️
terraform/node_resource_plan.go 93.44% <0.00%> (+1.63%) ⬆️

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Huh! Great catch/fix with that .Ptr()!

This is a somewhat new situation, so I have some thoughts about the error message and I'm curious what you think.

Including multiple "required_version" settings in a single module used to result in an error (with the exception of an override file); I think I'd expect a different error that the one shown here when there are multiple required_versions and terraform cannot chose a single version from those constraints, similar to the provider error (this is not the "real" error, but close):

This version of Terraform does not meet the constraints "~> 0.9.0, >= 0.13.0"

(possibly including locations for both constraints)

@alisdair
Copy link
Contributor Author

Thanks for the comment!

I think about what we're doing here differently than the provider installer case of choosing a version from the given constraints. Here we already have a single version (the current one), and we're verifying that it satisfies each constraint in turn.

If I had a configuration with multiple required_version definitions, and one of them was failing, I think I'd probably only want to be pointed at that one. It's easier to fix a single constraint you're pointed at, compared to having to also figure out which of the listed constraints doesn't pass before fixing it.

It would be nice to be able to require at most one Terraform version constraint (aside from overrides), but I think that would be a breaking change, so we can't do it until 0.14 at the earliest.

@mildwonkey
Copy link
Contributor

That sounds reasonable to me! I am sorry, I had meant to approve this as-is and not block you on that question. Thank you!

@apparentlymart
Copy link
Contributor

Oh, this exact brand of bug is the main reason why that .Ptr method exists! I made it many times during earlier HCL 2 and Terraform 0.12 work. I guess I missed this one. 🤦‍♂️ Thanks for tracking it down!

@alisdair alisdair merged commit 30c7dfc into master Aug 19, 2020
@alisdair alisdair deleted the alisdair/fix-required-version-diags branch August 19, 2020 15:26
@ghost
Copy link

ghost commented Oct 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants