Skip to content
This repository was archived by the owner on Jan 13, 2026. It is now read-only.

Fixes #3347 by not assuming semver for app version.#3348

Merged
absoludity merged 2 commits into
vmware-tanzu:masterfrom
absoludity:3347-appversion-not-semver
Sep 7, 2021
Merged

Fixes #3347 by not assuming semver for app version.#3348
absoludity merged 2 commits into
vmware-tanzu:masterfrom
absoludity:3347-appversion-not-semver

Conversation

@absoludity
Copy link
Copy Markdown
Contributor

Description of the change

As per the Helm docs for the Chart.yaml, the appVersion field is not required to be semver at all.

I wonder whether we should assume this for package version as well. For Helm it's safe, but we can't base our decisions on Helm. Furthermore, it's already enough that the backend has indicated that the latest version is different to the current version, so I can't see why we need the semver check here in the frontend even for the package version.

Let me know and I'll update to remove it there also.

Benefits

Possible drawbacks

Applicable issues

Additional information

Signed-off-by: Michael Nelson <minelson@vmware.com>
Copy link
Copy Markdown
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the fix. Yep, we assumed appVersion should be semver...

+1, but please add the fix also in this other view (ChartUpdateInfo):

https://github.com/kubeapps/kubeapps/blob/95064092e372a08002f8d8e92934e9796b9e25d9/dashboard/src/components/AppView/ChartInfo/ChartUpdateInfo.tsx#L16-L26

Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity
Copy link
Copy Markdown
Contributor Author

+1, but please add the fix also in this other view (ChartUpdateInfo):

Done. Also, did you see my comment above:

I wonder whether we should assume this for package version as well. For Helm it's safe, but we can't base our decisions on Helm. Furthermore, it's already enough that the backend has indicated that the latest version is different to the current version, so I can't see why we need the semver check here in the frontend even for the package version.

?

@absoludity absoludity merged commit 23bf64a into vmware-tanzu:master Sep 7, 2021
@absoludity absoludity deleted the 3347-appversion-not-semver branch September 7, 2021 03:46
@antgamdia
Copy link
Copy Markdown
Contributor

Ooops, I missed it, sorry!

I wonder whether we should assume this for package version as well. For Helm it's safe, but we can't base our decisions on Helm. Furthermore, it's already enough that the backend has indicated that the latest version is different to the current version, so I can't see why we need the semver check here in the frontend even for the package version.

Mmm, good point here... yep, it makes sense. Given that we are, actually, passing the lastest_version as part of the response, we could just rely on version != latest_version for this check. Besides, it is less Helm-centric, which is great. I'll send a PR fixing that. Thanks for pointing it out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid Version: latest

2 participants