Bug 1339754 - Fix tag sorting according to semantic versioning rules #9600
Conversation
| @@ -944,6 +948,20 @@ func PrioritizeTags(tags []string) { | |||
| minor = append(minor, v) | |||
| continue | |||
| } | |||
| case reMajorWithPatch.MatchString(short): | |||
There was a problem hiding this comment.
semver is only capable of working with fully specified version string iow. x.y.z with or without -patch part.
There was a problem hiding this comment.
semver is only capable of working with fully specified version string iow. x.y.z with or without -patch part.
You're saying their Parse is broken? That seems like the place to fix it.
There was a problem hiding this comment.
Not quite, they're stating to be compliant with semver.org which specifically says A normal version number MUST take the form X.Y.Z. The problem is we will deal with all sorts of version schemes, see for example the enclosed bug which uses 9.4-6, 9.4-6.1, 9.4-6.2, 9.4-6.4 which won't work not even with this patch :/ But it's almost impossible to support all possible versioning scheme. "Fixing" those missing Y or Z part is doable and we already are doing that. What I'm more worried here is that additionally we'll sort 9 < 9.4 < 9.4.1 due to the split, which worries me the most.
|
@smarterclayton bump |
| @@ -896,8 +896,12 @@ func LatestObservedTagGeneration(stream *ImageStream, tag string) int64 { | |||
| } | |||
|
|
|||
| var ( | |||
| reMajorSemantic = regexp.MustCompile(`^[\d]+$`) | |||
| reMinorSemantic = regexp.MustCompile(`^[\d]+\.[\d]+$`) | |||
| reMajorSemantic = regexp.MustCompile(`^[\d]+$`) | |||
There was a problem hiding this comment.
I don't think this is that important to support.
There was a problem hiding this comment.
You're suggesting supporting only x.y and ignore all the rest?
There was a problem hiding this comment.
The goal was to select only a subset - something realistic, but not be slavish on it. 9-9 doesn't really mean anything, 9.2-5 kind of means something, 9.3.5-6 is generally accepted to mean something. So only supporting minor with dash is probably ok, but even then, that's not full semantic versioning. I'd say x.y- and x.y.z-w are reasonable, the others are not.
Also, sorting order is really undefined on sub tags (rc1 is only coincidentally sorted above alpha1, so frankly i'm a little skeptical that we should let those be prioritized in numeric order).
|
I'm not really that interested in supporting non-semver. My opinion of the bug is that it's nice, but not supported. |
|
As in, when I originally did this I intentionally excluded non-semver. |
|
@smarterclayton you are supporting partially semver, you're suggesting to drop it entirely? |
|
Discussed on IRC, the final saying is, quoting @smarterclayton:
|
|
Updated to satisfy previous comments. @smarterclayton good to merge? |
|
@smarterclayton ping? |
|
@smarterclayton if I hear no objections I'd like to merge it later today |
|
@smarterclayton @soltysh do we have any updates on this, it looks like this can be merged? |
|
@sferich888 I think we can safely merge it, but only when 1.5 merge queue opens. |
|
[merge] |
|
Rebased and fixed the test, should be good to merge. |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11608/) (Image: devenv-rhel7_5394) |
|
Fixed another test in integration. |
|
Evaluated for origin merge up to bff7cab |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to bff7cab |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11608/) (Base Commit: 1f1e6de) |
Partially fixes bug 1339754 in that it correctly reads patch part from non-full versions (eg.
1.1-beta1).@smarterclayton ptal and help me understand why our current sorting algorithm prefers
9.5over9.5.1. I know the former isn't fully specified semver but since we're expanding it to make it through semver parsing why not sorting it accordingly?I'd love to live in a perfect world, but we're not and we need to deal with problems as such from the bug I'm trying to solve. Although that bug is too far from semver we support.