Skip to content
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

Sort packages by version instead of relying on ChartVersions[0] #6588

Merged
merged 5 commits into from Aug 9, 2023

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Aug 8, 2023

Description of the change

As described in #6584, we were returning wrong versions in some corner cases (mainly using pre-releases). It seems we were relying on ChartVersions[0] with the explicit assumption that it would always contain the latest version. However, this is not the case. Since the DB does not know about semver (unless we used some extensions), the ORDER BY query might be wrong.

This PR is replacing the occurrences of ChartVersions[0] in favor of a previous semver sorting. The affected operations are: GetAvailablePackageDetail, GetAvailablePackageSummaries, GetInstalledPackageSummaries and GetInstalledPackageDetail.

Benefits

The latestVersion returned by the API (and therefore used by the UI -as is-, we don't sort them in client-side) will be the one that has to be.

Possible drawbacks

  1. In this PR I have just tacked the Helm plugin, not the Flux one.
  2. In the code we have relied on this assumption, not 100% of the side effects, if any.

Applicable issues

Additional information

Example of a new version being properly detected:

image

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@netlify
Copy link

netlify bot commented Aug 8, 2023

Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 6653572
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/64d275689134e900086610eb

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Copy link
Contributor Author

antgamdia commented Aug 8, 2023

I have noticed (c.f. #6249) some scenarios where the versions are not semver. In those cases, I have just fallen back to the previous behavior. However, I wonder if we should replace the sorting just with a lexicographic order, instead of blindly relying on ChartVersions[0]. See what you think.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks Antonio. I was thinking the the issue may be related to the way chartmuseum was creating the index, when app versions aren't present, but realised it's GAR (OCI) so not relevant. Either way, this is much better than our previous reliance on the ordering in an index (or even in the sync code that we have for OCI repos).

@antgamdia antgamdia merged commit 15173e4 into vmware-tanzu:main Aug 9, 2023
39 checks passed
@antgamdia antgamdia deleted the 6584-sortPkgByVersion branch August 9, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Artifact Registry-based helm chart is not defaulting to latest version
3 participants