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

internal: Suppress duplicate version constraints #26678

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

alisdair
Copy link
Contributor

A set of version constraints can contain duplicates. This can happen if multiple identical constraints are specified throughout a configuration.

When rendering the set, it is confusing to display redundant constraints. This commit changes the string renderer to only show the first instance of a given constraint, and adds unit tests for this function to cover this change.

This also fixes a bug with the locks file generation: previously, a configuration with redundant constraints would result in this error on second init:

Error: Invalid provider version constraints

  on .terraform.lock.hcl line 6:
  (source code not available)

The recorded version constraints for provider
registry.terraform.io/hashicorp/random must be written in normalized form:
"3.0.0".

Fixes #26670.

A set of version constraints can contain duplicates. This can happen if
multiple identical constraints are specified throughout a configuration.

When rendering the set, it is confusing to display redundant
constraints. This commit changes the string renderer to only show the
first instance of a given constraint, and adds unit tests for this
function to cover this change.

This also fixes a bug with the locks file generation: previously, a
configuration with redundant constraints would result in this error on
second init:

Error: Invalid provider version constraints

  on .terraform.lock.hcl line 6:
  (source code not available)

The recorded version constraints for provider
registry.terraform.io/hashicorp/random must be written in normalized form:
"3.0.0".
@alisdair alisdair requested a review from a team October 22, 2020 16:09
@alisdair alisdair self-assigned this Oct 22, 2020
@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #26678 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
internal/getproviders/types.go 66.34% <100.00%> (+29.34%) ⬆️
terraform/eval_apply.go 74.01% <0.00%> (-0.61%) ⬇️
terraform/evaluate.go 52.91% <0.00%> (+0.41%) ⬆️
terraform/node_resource_plan.go 97.19% <0.00%> (+1.86%) ⬆️
internal/providercache/dir.go 71.42% <0.00%> (+6.12%) ⬆️

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.

Looks great, thanks!

This did get me thinking about whether we should also try to sort the components in some reasonable way, continuing the theme of there being one canonical way to write constraints. I think right now there will be a consistent ordering that comes indirectly from Terraform reading out the underlying dependency values from the configuration in a particular way, but now that we are deduping across modules it feels weird that the result would still depend on the way the modules are declared.

What do you think? Would it be reasonable to sort the constraint items maybe first by the operator and then by the version number, so that the widest bounds get pushed out to the edges? Not suggesting that we must do that in this PR, but if it seems sensible to you then we could do it in a separate PR before the 0.14.0 final release.

@alisdair
Copy link
Contributor Author

Yes, it would be nice to have a consistent string output for a set of version constraints, especially if we could make it clear to read. I had a quick look at implementing it and it doesn't feel trivial enough for this PR, but it would be good to come back to if we have time.

@ghost
Copy link

ghost commented Nov 26, 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 Nov 26, 2020
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.

Finding provider plugins prints same version number repeatedly
2 participants