Set the LatestMatchingVersion field properly
#4067
Conversation
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
| @@ -612,7 +612,7 @@ func (s *Server) UpdateInstalledPackage(ctx context.Context, request *corev1.Upd | |||
| } | |||
|
|
|||
| // Update the rest of the fields | |||
| pkgInstall.Spec.PackageRef.VersionSelection = &vendirVersions.VersionSelectionSemver{Constraints: pkgVersion} | |||
| pkgInstall.Spec.PackageRef.VersionSelection = &vendirVersions.VersionSelectionSemver{Constraints: fmt.Sprintf(">=%s", pkgVersion)} | |||
There was a problem hiding this comment.
Hmm, I thought we'd used pkgVersion here for safety (so people don't get auto-upgrades to new major versions) until we support users specifying the version selection in the UX? I mean, won't this change mean that users will now have no choice and always end up with the latest version? That is, if I specifically installed 2.3 of something, because I don't want to run 3.0, the version constraint here will be set to >=2.3 so that after my initial install it'll get upgraded automatically to 3.0 (or whatever is the latest)?
Or am I missing something?
There was a problem hiding this comment.
Ooops, you're totally right (I often forget about the Carvel auto-updates). Given the way we are using the version constraints for getting a new matching version, if we set it to constraints: x.y.z (that is, selecting just the exact version x.y.z) we will never get a matching version.
Don't know if we could switch instead to ~1.2.3 (will use releases from 1.2.3 to <1.3.0) or ^2.3.4 (will use releases from 2.3.4 to <3.0.0).
However, happy to leave it unchanged (that is, reverting the >= change back) and decide what to do in the future (when we support it in the UI, as you said)
There was a problem hiding this comment.
Don't know if we could switch instead to
~1.2.3(will use releases from 1.2.3 to <1.3.0) or^2.3.4(will use releases from 2.3.4 to <3.0.0).
Nice, yes if ~1.2.3 works (ie. automatic patch updates), I think that'd be an improvement over what we have now (no updates at all), without being unexpected, dangerous. And we can document how it can be changed via kubectl for now until we have a UI for it, but otherwise, yes, I'd leave it unchanged. I just don't remember seeing any examples in the carvel docs using the "approximately equivalent to version" operator (~), but I guess we could even generate >1.2.3 <1.3.0 format (which is in the carvel docs).
There was a problem hiding this comment.
I've just checked it; unfortunately, there is no ~ operator (Reconcile failed: Selecting versions: Parsing version constraint ''~1.17.1'': Coul...) so the only way for us to support this is generating the equivalent as you mentioned. It worths a separate PR (so that we can make this behavior configurable (eg, autoupgrade: major|minor|patch|none))
Edit: done #4085
I'm +1 on the actual change that updates and displays the latest matching version...
I guess you didn't actually click on the +1 gh button, no?
| @@ -187,6 +187,11 @@ func (s *Server) buildInstalledPackageSummary(pkgInstall *packagingv1alpha1.Pack | |||
| iconStringBuilder.WriteString(pkgMetadata.Spec.IconSVGBase64) | |||
| } | |||
|
|
|||
| latestMatchingVersion, err := latestMatchingVersion(versions, pkgInstall.Spec.PackageRef.VersionSelection.Constraints) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Cannot get the latest matching version for the pkg %q", pkgMetadata.Name) | |||
There was a problem hiding this comment.
We should log the err here I think? (Same below, rather than swallowing the err completely)
| @@ -418,7 +435,7 @@ func (s *Server) buildPkgInstall(installedPackageName, targetCluster, targetName | |||
| PackageRef: &packagingv1alpha1.PackageRef{ | |||
| RefName: packageRefName, | |||
| VersionSelection: &vendirversions.VersionSelectionSemver{ | |||
| Constraints: pkgVersion, | |||
| Constraints: fmt.Sprintf(">=%s", pkgVersion), | |||
There was a problem hiding this comment.
See earlier comment about this being the default.
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
There was a problem hiding this comment.
I guess you didn't actually click on the +1 gh button, no?
No, I hadn't. Done now. For the future, no need to block on that, as in, feel free to grab another team member to just confirm you've addressed the issue, then a +1 from them to land. That's why I don't click on the Request changes generally.
Description of the change
This PR allows setting the proper value for the
LatestMatchingVersion. Initially it was set to the latest version, but it should take into account the version constraint the PackageInstall has been created with.To do so, a simple function is added that, assuming a sorted array of package versions, returns the first one matching the semver version constraint.
By default, the version constraint when installing a package in Kubeapps is
>= pkgVersionBenefits
The field will have the semantics it is expected to have.
Possible drawbacks
N/A
Applicable issues
Additional information
N/A