loosen version verification #3750
Conversation
| if versionRe.MatchString(version) && req.Resolved.ingredient != nil { | ||
| if req.Resolved.ingredient != nil { |
There was a problem hiding this comment.
The regex does seem like it was overkill here, but reading this code I think we still want to check if version is non-empty? In most cases we run state install without a specific version, in which case this it doesn't seem like we should run this logic.
There was a problem hiding this comment.
hmm, I ran a single test of state install namespace:name (no dynamic, no version) and that worked. But I would be okay to check for the empty string.
| func isExactVersion(version string) bool { | ||
| return versionRe.MatchString(version) | ||
| func isRangeVersion(version string) bool { | ||
| return strings.Contains(version, "=") || strings.Contains(version, "<") || strings.Contains(version, ">") |
There was a problem hiding this comment.
strings.Contains(version, "=")
This doesn't indicate a range version does it? I suppose you could have >= but = alone could also apply to ==1.0.
There was a problem hiding this comment.
no, = alone doesn't indicate a version range, but == is a range, as are >=, <=, !=, and others that are less common like ~=. It is just an indicator, as = is never legal in a concrete version identifier.
these three characters are the minimum sufficient to determine if any of the universal version range operators are being used.
There was a problem hiding this comment.
Sorry I don't think I'm following, let me ask a different way;
Wouldn't isRangeVersion('==1.0') return true even though it should be false?
There was a problem hiding this comment.
That one is a bit different, true, but IMO it does and should return true. There is a distinction between ==1.0 and 1.0, though maybe not to the state tool. The former is a constraint and does represent a range, but a range that includes only one version; the latter is a versions string.
So the practical reason to send even ==1.0 to the back end is that == should not be part of the version string. Sending it to the platform means we don't have to special case this one operator for stripping. This is good, as version strings get complex across so many ecosystems, e.g. in Rust = is the eq operator, not ==.
Co-authored-by: Nathan Rijksen <[email protected]>
3 participants
This change comes down to removing the regex checks on user-supplied version string. They were overly constraining. Now everything is allowed to at least be attempted on the platform. If it really can't be a version, then an error will come back from the platform; probably a solve error.