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
Fail global required_version check if it contains any prerelease fields #31331
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5220c4c
Fail global required_version check if it contains any prerelease fields
liamcervante df90f74
go mod tidy
liamcervante 6f423c1
Improve required_version prerelease not supported error string
liamcervante b5e1794
Add prerelease version constraint unit tests
liamcervante 2b5fb66
Fix side-effects by populating global diags too soon
liamcervante File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think we'll skip all constraints after the first one fails for any reason, whereas at the moment I think we check all of the constraints that we can. Could we change this to only
continue
here if the current constraint has a prerelease version in it, allowing other constraints to evaluate separately?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to clarify.
At the top level we have the
CoreVersionConstraints
field.CoreVersionConstraints
is a slice ofVersionConstraint
.VersionConstraint
is just a wrapper around the go-versionConstraints
struct (with some metadata).Constraints
is then a slice ofConstraint
structs.The existing behavior and my implementation both will only ever print a single diagnostic for each
VersionConstraint
and both iterate over the entireCoreVersionConstraints
slice and analyse all of them even if an early one fails.I think what you're asking for is this: #31339. But I would argue it represents a greater behavior change than my current implementation in this PR.
More details:
The existing behavior eagerly fails within
Constraints
. As soon as one of the constraints doesn't match the version it gives up.CheckCoreVersionRequirements
will then add a single diagnostic for the entireVersionConstraint
that wrapped the currentConstraints
struct and then move on to the nextVersionConstraint
in theCoreVersionConstraints
field.My implementation (this PR) matches this, so if a single
Constraint
contains a prerelease field then it will eagerly fail without checking the rest of the constraints within that specificVersionConstraint
and add a single diagnostic for thatVersionConstraint
. But it will then continue on and still check all the otherVersionConstraint
structs within theCoreVersionConstraints
field and print out a specific diagnostic for each one.The other PR changes this behaviour so that every
Constraint
within eachVersionConstraint
is always evaluated instead of just giving up when the first one fails.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying this branch locally seems to show a change in behaviour which lines up with my reading of the code. Consider this (nonsense) configuration:
On main, running
terraform init
gives two diagnostics:On this branch, only the first constraint diagnostic is shown (because it's added to
diags
, and we thencontinue
past any others in this module):What I was trying to suggest was something like this:
That is, keeping track of prerelease diags separately from others, and only skipping over constraint checking for this iteration of the loop if there's a prerelease present.
I don't think this is a critical behaviour change, and my conditional approval still stands if this doesn't make sense to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see it now! Apologies and fixed!