Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses incorrect version sorting for Helm charts by implementing a new compareChartVersions helper function that properly handles Rancher's special "up" build metadata and standard semver comparisons. The change fixes issue #14817 where release candidate versions were being displayed out of order on the chart detail page.
Changes:
- Added
compareChartVersionsfunction to handle semver comparison with special logic for Rancher's "up" metadata - Updated chart version sorting in
mappedVersionscomputed property and upgrade/downgrade action determination - Removed obsolete
isUpgradeFromPreToStablehelper function and its tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shell/utils/chart.js | Added new compareChartVersions function with semver comparison and Rancher "up" metadata handling |
| shell/mixins/chart.js | Updated to use compareChartVersions for sorting versions and determining upgrade/downgrade actions |
| shell/utils/version.js | Removed obsolete isUpgradeFromPreToStable function |
| shell/utils/tests/version.test.ts | Removed tests for deleted isUpgradeFromPreToStable function |
| shell/mixins/tests/chart.test.ts | Added comprehensive test coverage for new sorting logic and action determination with various version formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param {string} v2 - The second version string. | ||
| * @returns {number} - 0 if equal, -1 if v1 < v2, 1 if v1 > v2. | ||
| */ | ||
| export function compareChartVersions(v1, v2) { |
There was a problem hiding this comment.
The new compareChartVersions function lacks direct unit tests. While it's tested indirectly through the chart.test.ts tests for mappedVersions and action, consider adding dedicated unit tests in shell/utils/__tests__/chart.test.ts to cover edge cases such as null/undefined inputs, malformed versions, and various 'up' metadata patterns.
There was a problem hiding this comment.
This one would be good to do. It's a granular function so having explicit tests for the issue would help
|
@richard-cox I addressed the initial feedback from Copilot but then I mistakenly re-requested a review which caused it to came back with the current code change requests that I think are not necessary. Let me know what you think. |
richard-cox
left a comment
There was a problem hiding this comment.
Just some minor tweaks needed.
FWIW i couldn't find any rancher or partner charts with RC now (that are applicable to the latest version of rancher). in the end i just used our rancher latest repo
| * @param {string} v2 - The second version string. | ||
| * @returns {number} - 0 if equal, -1 if v1 < v2, 1 if v1 > v2. | ||
| */ | ||
| export function compareChartVersions(v1, v2) { |
There was a problem hiding this comment.
This one would be good to do. It's a granular function so having explicit tests for the issue would help
shell/utils/chart.js
Outdated
| let diff = semver.compare(v1, v2, { loose: true }); | ||
|
|
||
| if (diff === 0) { | ||
| const vC = semver.parse(v1, { loose: true }); |
There was a problem hiding this comment.
v + c are still used throughout this method, instead of v1 and v2
There was a problem hiding this comment.
sorry I misread your earlier comment on this, done.
Summary
Fixes #14817
Occurred changes and/or fixed issues
We were relying on the order of the versions we received from the API but now we apply sorting to make sure the order is always correct.
Technical notes summary
compareChartVersionshelper that usessemverlibrary:semver.compare).+upX.Y.Z). If found, it strips "up" and compares the inner versions recursively. This ensures...-rcversions inside "up" are correctly treated as older than stable.semver.compareBuildto sort alphabetically.shell/mixins/chart.jsto usecompareChartVersionsfor both sorting versions (mappedVersions) and determining the action(e.g. upgrade/downgrade...).isUpgradeFromPreToStablehelper fromutils/version.jsas it is now covered by the new logic.shell/mixins/__tests__/chart.test.tsto cover various scenarios.Areas or cases that should be tested
...-rc).Areas which could experience regressions
Screenshot/Video
Checklist
Admin,Standard UserandUser Base