-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Masterminds/semver and put charts into a chrono order #171
Use Masterminds/semver and put charts into a chrono order #171
Conversation
162f7d8
to
0da4fc0
Compare
9267a9f
to
8b72327
Compare
wantErr: true, | ||
}, | ||
{ | ||
name: "do not match pre-release", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8b72327
to
3018f58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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/semver
for two reasons:cc @hiddeco @stefanprodan