-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Allow successful init when constraint matches at least one valid version #37137
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
Conversation
8e22984
to
159f1a3
Compare
159f1a3
to
a095350
Compare
What you do think about including a warning (in the main output, not just logging) when invalid responses were returned from the registry? Something like "the registry returned invalid response X for provider Z so we selected the best of the valid options" (but better words)? I'm picturing a user who expected a provider upgrade and doesn't understand why the version they can see in the registry isn't downloading (it's there in the logs, so this is really just a thought, not something we need immediately or at all!) |
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.
The implementation looks solid, but I feel like terraform should emit a warning (not just log the error) when encountering invalid responses from the registry, so it's immediately obvious to the user that there's something going on even if terraform can continue. (Not a merge blocking thought, I'm curious what others thing)
The issue with doing this is that if a provider has a large number of invalid versions, most of which were skipped, and thus having nothing to do with the final selected version, the output still contains a warning for those versions. Also, I feel like the provider warnings we have today are indications of things that the user can fix, e.g upgrading to a non-deprecated version or resource. However, in this case, there is nothing the user can do, and they probably do not care, because their constraint still resolved to a version. |
Warnings are usually emitted for something that the practitioner can take action on, but the registry database containing an invalid value isn't something that any Terraform user can fix. The warnings in this part of the code are meant for a different use case, the registry can return messages about using a specific version, so the user can change the requirements set in the configuration. The reasoning I gave @dsa0x, was that in the rare case of something failing in the registry which causes the expected version to be absent here, then it would be clear the failure is somewhere between Terraform and the provider release process. Checking the debug logs from the CLI, and contacting the registry would be the next steps regardless of the CLI output. maybe we could overload the warnings from this code path, but it might not be worth the change. |
That's fair, no objections from me! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on th
84EA
e active contributions. |
This makes s slight change to the
init
command, so that any version from the registry that does not conform to terraform's semver constraints is skipped, allowing the command to use other versions if present. If there is no version deemed valid, the command still appropriately gets an error, but the error would likely tell the user that the registry has no version that matches the requested constraint.Target Release
1.13.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry