replicatedhq
Support semver
@areed @divolgin Hi, I added the possibility to compare semversions. The group containing the version must be called "version" or "Version". If it is the case, the text analyzer runs a semver analysis. Fix #275.
@manavellamnimble there's a little too much "magic" in this design. How can let the author of the spec declaratively state that they want a semver comparison?
Currently, this let's me write: version < 1.0.0
I'd like to be able to just write a range using standard (npm style?) semver ranges like >= 1.19.0 < 1.23.9
Maybe instead of overloading the when fields, we should consider introducing a new field to specify the comparison method.
- fail:
when: '< 1.19.0'
compare: semver
or something similar. Let's work on the design here.
@marccampbell I think this can be don in the PR as is. We can try calling semver.ParseRange on the when, and if it fails, fall back on current behavior. I like compare though.
Alternatively to compare, when could be an object that unmarshals into a semver, int, string struct or something.
@marccampbell this pr would work as you propose, eg.:
regexGroups: 'Version (?P<version>\d+\.\d+.\d+)'
outcomes:
- fail:
when: 'version < 1.19.0'
message: Your Kotsadm version does not meet required minimum version.
The flow in the analyzer would be:
- if capturing group name == 'version' -> semver comparison
- else if the value side of the when statement is int -> int comparison
- else -> string comparison
I can add a stage to parse the when statement to an int, semver or string and select which comparison case fits best.
@marccampbell I've worked in a more intuitive alternative, troubleshoots decides which comparison case to use based on the found value and the value in the when statement.
I am not against adding the field compare, if you think that making the specs more declarative is better, I can add the field. But maybe from the user side adding a compare statement in each possible outcome may be a bit tedious.
@markpundsack what do you think here? automatically determining int/string/semver is nice, but it can lead to cases where someone what's to do a string comparison, but it's a valid semver, and we end up doing something unexpected.
Generally, I think being declarative and specific about the intent is normally better, but I don't want to require additional fields that might introduce errors and misconfiguration.
Yeah, I love implied stuff which just does the right thing, but it's horrible when it's wrong. Explicit usually wins there. At the very least, there needs to be a way to override the implicit understanding. I don't know what that means in this particular case.
Prior art? http://masterminds.github.io/sprig/semver.html
@marccampbell @markpundsack watching what Mark shared and thinking in making it imperative, I've rewritten it, and it would work like this:
regexGroups: 'Version (?P<my_variable>\d+\.\d+.\d+)'
outcomes:
- fail:
when: 'semverCompare my_variable < 1.19.0'
message: Your Kotsadm version does not meet required minimum version.
I've written it in a way which would allow to add further comparison strategies or methods.
I'm not sure I understand the intent here. I'm glad that "semverCompare" is explicit. But it seems weird that in one place I need to write an explicit regex for parsing a version, but in the comparison I don't reference any of those parts. Is that regex really needed to be that complex? In fact, it's brittle because it requires 2 decimal points. What if the Version was 1.19, the regex itself would fail. Maybe it would work with a different regex and it's just a bad example.
Do we support compound conditions? Adding "semverCompare" might be OK in the example (although it's not exactly intuitive), but if there were other conditions, can someone add brackets around it like "semverCompare(my_variable < 1.19.0) && semverCompare(my_variable > 1.16.0)"? Without it, it would be ambiguous. If that's not something we support generally, but would often need for semver, we could have users create multiple when conditions, and fail for each separately. But maybe we could also do semverCompare 1.16.0 <= my_variable < 1.19.0.
I wonder how we could handle something as complex as this:
semver.satisfies('1.2.3', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')
That last example makes me wonder if simple < comparison is not the right goal, but doing a semver "match" is better. Test whether the given version does or does not meet a semver spec.
@markpundsack I could work on more complex ranges for the semver comparison. Actually I only considered the usual operators (<,<=,>...) based on the issue description and the comparison used for the mysql and postgre analyzer. But the library allows specifying ranges using logial OR or AND operations: https://godoc.org/github.com/blang/semver/v4#ParseRange.
We could use semver.satisfies(variable, range) as suggested, eg. semver.satisfies(my_variable, "1.2.3 1.x || >=2.5.0 || 5.0.0 - 7.2.3")
semver.satisfies was just an example from that npm module. I wouldn't suggest it for our uses, especially because it's an object delimiter which doesn't make sense in this context. But we could use something like semverRange(variable, range). People might find the standard < comparison to be easier to get started. semverRange(my_variable,"<1.19") is a little more awkward than semverCompare(my_variable < 1.19). Maybe support both? Having said that, we don't need to support both now. We could start with the compare, because it's pretty useful as-is, and you can already cover multiple cases with multiple when clauses, so range may not be necessary. I'm just now debating between the () syntax and your proposed syntax.
It's also a little awkward that we have a different syntax here vs the MySQL/Postgres analyzers. It makes sense, given the latter are explicitly about versions, but if there were an easy reconciliation, it would be nice.
@markpundsack new version, supporting:
- semverCompare(variable operator version), for straightforward <,>,=... comparison
- semverRange(varible, range) for complex range expresions, supporting the expressions described here
I used regex to extract valid expressions inside the parenthesis and parsed conditionals into a struct to make a coherent use of it in the analyzer.
This contains a change to text_analyze.go which could still be relevant and is not currently implemented, but due to the discussion in this PR plus the need to rebase and resolve conflicts, I've marked it as draft.
