Use Masterminds/semver and put charts into a chrono order #171
Conversation
wingsofovnia
force-pushed
the
feauture/chrono-order-ambigous-matches
branch
2 times, most recently
from
162f7d8
to
0da4fc0
Compare
wingsofovnia
force-pushed
the
feauture/chrono-order-ambigous-matches
branch
2 times, most recently
from
9267a9f
to
8b72327
Compare
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "do not match pre-release", |
wingsofovnia
force-pushed
the
feauture/chrono-order-ambigous-matches
branch
from
8b72327
to
3018f58
Compare
There was a problem hiding this comment.
Thanks for putting time into this 🌻
We have not made a final decision yet on what action to take for the problem described in #127. Both libraries have their up and downsides (and pitfalls), as you can see in the comments I made.
There is an additional problem this PR introduces, as it creates a backwards incompatible change due to blang/semver not supporting the same version ranges as Masterminds/semver (see https://play.golang.org/p/D5oA-MpHggJ for an example). Which means we can not lightheartedly, and without a proper discussion (including input from the community) introduce this, as some people may be relying on the existing logic.
In addition, for the added support for build metadata I would like to have seen a proposal first. This way, people in the community can give feedback and/or counter proposals before introducing something new.
| @@ -220,7 +220,7 @@ var _ = Describe("GitRepositoryReconciler", func() { | |||
| reference: &sourcev1.GitRepositoryRef{SemVer: "v1.0.0"}, | |||
| waitForReason: sourcev1.GitOperationFailedReason, | |||
| expectStatus: corev1.ConditionFalse, | |||
| expectMessage: "semver parse range error", | |||
| expectMessage: "no match found for semver: v1.0.0", | |||
There was a problem hiding this comment.
The reason you had to change this is because it introduces a new bug, see: fluxcd/flux#2729.
It can be fixed by using semver.StrictNewVersion, as described in: fluxcd/image-reflector-controller#22 (comment), but this creates a new problem for Git repository tags as the prefix may have to be stripped from the tag first (and added back later).
| if !latestStable { | ||
| rng, err := semver.ParseRange(version) | ||
| // Parse version constraints if given (default strategy: latest) | ||
| var versionLatestStable = len(version) == 0 || version == "*" |
There was a problem hiding this comment.
Better to fall back to semver.NewConstraint("*"), to piggy back on the library's logic.
| // Check for exact matches first | ||
| if version == chartVersion.Version { | ||
| return chartVersion, nil | ||
| } |
There was a problem hiding this comment.
Checking for exact matches first is (without having benchmark proof) likely much faster, as semver.NewVersion / semver.StrictNewVersion do a lot of parsing.
| return chartVersion, nil | ||
| } | ||
|
|
||
| version, err := semver.NewVersion(chartVersion.Version) |
There was a problem hiding this comment.
This should be semver.StrictNewVersion for reasons given above.
|
Thank you for the detailed review. Indeed, there changing the lib can drop support in some cases, but given that the new lib is the one that is used in Helm itself, I assumed predictability and alignment is more preferred. However, I agree a broader discussion is needed. As to the build metadata, I haven't added any extra support for it as it is supported by blang/semver already. What is new is only an order of equally (with the meaning of Semver) versioned Charts that overwise is not specified by spec. At that point, I plan to create a new PR for only that a question of lib migration will take more time. |
wingsofovnia
changed the title
In the case of an ambiguous chart version match, take the most recently created chart.
This is especially valuable for dev environments where it is common to deploy charts of the same (major.minor.patch) version but coming from different builds (hence different build metadata, as per Semver para 10). In such a setup, metadata usually contains build (git) revision that makes the default strategy of lexicographical sorting to pick unpredictable charts.
The Semver spec neither defines nor restricts particular order for such cases so this change doesn't violate the spec.
I also migrated to
Masterminds/semverfor two reasons:cc @hiddeco @stefanprodan