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
Changes from 2 commits
5220c4c
df90f74
6f423c1
b5e1794
2b5fb66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,25 @@ func CheckCoreVersionRequirements(config *configs.Config) tfdiags.Diagnostics { | |
module := config.Module | ||
|
||
for _, constraint := range module.CoreVersionConstraints { | ||
// Before checking if the constraints are met, check that we are not using any prerelease fields as these | ||
// are not currently supported. | ||
for _, required := range constraint.Required { | ||
if required.Prerelease() { | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "Invalid required_version constraint", | ||
Detail: fmt.Sprintf("Prerelease version constraints are not supported: %s.", required.String()), | ||
Subject: constraint.DeclRange.Ptr(), | ||
}) | ||
} | ||
} | ||
|
||
if len(diags) > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to clarify. At the top level we have the The existing behavior and my implementation both will only ever print a single diagnostic for each 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 My implementation (this PR) matches this, so if a single The other PR changes this behaviour so that every There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: terraform {
required_version = "> 1.3.0"
}
terraform {
required_version = "< 1.3.0"
} On main, running $ terraform init
╷
│ Error: Unsupported Terraform Core version
│
│ on main.tf line 2, in terraform:
│ 2: required_version = "> 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵
╷
│ Error: Unsupported Terraform Core version
│
│ on main.tf line 6, in terraform:
│ 6: required_version = "< 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵ On this branch, only the first constraint diagnostic is shown (because it's added to $ terraform init
╷
│ Error: Unsupported Terraform Core version
│
│ on main.tf line 2, in terraform:
│ 2: required_version = "> 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵ What I was trying to suggest was something like this: var prereleaseDiags tfdiags.Diagnostics
// Before checking if the constraints are met, check that we are not using any prerelease fields as these
// are not currently supported.
for _, required := range constraint.Required {
if required.Prerelease() {
prereleaseDiags = prereleaseDiags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid required_version constraint",
Detail: fmt.Sprintf(
"Prerelease version constraints are not supported: %s. Remove the prerelease information from the constraint. Prerelease versions of terraform will match constraints using their version core only.",
required.String()),
Subject: constraint.DeclRange.Ptr(),
})
}
}
diags = diags.Append(prereleaseDiags)
if len(prereleaseDiags) > 0 {
continue
} 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I see it now! Apologies and fixed! |
||
// There were some prerelease fields in the constraints. Don't check the constraints as they will | ||
// fail, and the diagnostics are already populated. | ||
continue | ||
} | ||
|
||
if !constraint.Required.Check(tfversion.SemVer) { | ||
switch { | ||
case len(config.Path) == 0: | ||
|
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'm wondering if this should also suggest the fix of removing the prerelease information and that the version constraint may still match the prerelease?
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, that seems reasonable. I've added the extra information to the error message.