Don't panic if the new version constraint parser is stricter than the old one #25223
Conversation
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
The new provider installer code is using a new version constraint parser because it produces better error messages than the one we were using before. However, it has some cases where it returns errors that the old parser (which was entirely regex-match-based) didn't catch. In the long run we should consistently use the new parser everywhere, but until then we'll avoid panicking then the two disagree, by returning diagnostic messages instead of using MustParseVersionConstraints. For now, we only hit these error cases if the user enters something that the old parser allows but the new parser does not.
This new version permits omitting the space between the operator and the boundary in a ruby-style version constraint, like ">1.0.0" instead of "> 1.0.0".
Codecov Report
|
alisdair
approved these changes
Jun 12, 2020
mildwonkey
approved these changes
Jun 12, 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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The new provider install logic for 0.13 uses a different version constraint parser that makes an effort to produce user-actionable error messages when the user enters something invalid or non-sensical, but as of right now the
configspackage is still in an in-between state between old and new because we didn't change the other places where version constraints appear to use the new parser yet.The new parser is intentionally a little stricter than the old one, because it's no longer just a regex match and aims to give feedback when it finds something weird, but it was inadvertently a little too strict in that it was enforcing the space between the operator and the boundary like in
~> 1.0.0, due to a mistaken impression on my part that this was required by rubygems (which our constraint syntax is based on). Turns out that rubygems does accept the space being absent and that's just not mentioned in any of the documentation.So with all of that said, there are two different things in this PR:
Accept a constraint like
~>1.0.0as equivalent to~> 1.0.0, which is non-idiomatic but also a pretty harmless thing to be making a fuss about. (That's achieved by upgrading the upstream dependency containing the parser.)If the input contains any other case where the new parser is stricter than the old (which should, after the change mentioned in the previous item, be much more marginal things that would be unlikely to appear in real configuration), return it as a proper error message rather than a panic.
I did consider going all the way to switching over to the new parser for other cases here, but that ended up being too invasive a change to make this close to final release, and so instead this is a short-term robustness improvement, focused only on avoiding panics, with the intent of using the new parser exclusively in a future release.
This closes #25177.