fix incorrect tag selection in arkade chart upgrade #940
Conversation
|
Could you also show that upgrade still works? Try these as examples: And: This one is going to be more complex: Technically, it should increase to a stable version. So when moving from an rc suffix to a stable suffix, that that is newer. |
As implemented it doesn't move from an rc suffix to a newer stable suffix. I would work on this, add extra test cases and update this PR accordingly. Thank you Alex |
|
Thanks 👍 |
| } | ||
| } | ||
| } | ||
|
|
| } | ||
|
|
||
| latest := vv[0] | ||
| if latest.Prerelease() != "" { |
There was a problem hiding this comment.
Could you extract to a small function with a self-describing name and a comment, or add a comment?
There was a problem hiding this comment.
I extracted the logic to for retrieving the latest stable version into its own function i.e getLatestStableVersion. I also added a comment to the block of code that loops through the list of versions to return find the latest stable version in case an unstable version is the selected as the latest. I felt I had addressed your comments with these changes. Please let me know if there was anything I misunderstood so I can send your recommended changes in an updated PR.
cmd/chart/upgrade_test.go
Outdated
| func Test_ChartUpgrade(t *testing.T) { | ||
| tests := []test{ | ||
| { | ||
| name: "builder_v11_rootless", |
There was a problem hiding this comment.
The name could be written in english..
"v11_rootless" doesn't help a contributor or maintainer who comes here 1 year later (or perhaps even yourself) with figuring out what this test case does.
cmd/chart/upgrade_test.go
Outdated
| expected: "moby/buildkit:v0.11.6", | ||
| }, | ||
| { | ||
| name: "prom_Stable", |
There was a problem hiding this comment.
I saw you excluded these unit tests..
Why? Is it because they're integration tests which actually call out to the Internet?
How about providing a canned list of available tags here in a similar format to what crane returns.
See the unit testing section of my eBook that I gifted to you.
You should see a pattern for stubbing out data.
There was a problem hiding this comment.
name: "Promote from a stable tag to the next stable tag"
current: "prom/prometheus:v2.42.0"
want: "prom/prometheus:v2.45.0"
tags: []{ "prom/prometheus:v2.42.0", "prom/prometheus:v2.42.0-rc1", "prom/prometheus:v2.44.0", "prom/prometheus:v2.45.0" }
etc
There was a problem hiding this comment.
Yeah I did that because they called out to the internet. But providing a canned list of available tags and testing against that would be better. I'd work on this and send an updated PR by the end of the day
|
Hi @bxffour what do we have in the latest changes? Is there anything to update in the PR description too? Thanks |
In this PR update, I have made several changes based on the provided suggestions:
|
bxffour
force-pushed
the
fix/issue-936
branch
2 times, most recently
from
0ef7287
to
cfcc607
Compare
|
@welteki could you take a look at the PR and review please? |
Is there a unit test that would have previously failed with the old logic that you can create or add in? That way we will protect against regression in the future if someone's confused as to why the logic works this way and changes it to replace all instead of line-by-line. |
No there isn't. I'd work on adding one asap |
Signed-off-by: Nana Kwadwo <agyemang.nana.b@gmail.com>
Thanks for your feedback. I'd replicate the problem and submit an updated PR asap. |
Hi @nitishkumar71, Sorry, it took me way too long to get to this. I think I might need a bit more information on your issue. I wrote a test case for the buildkit problem and my test passed. Does this test case reflect the conditions for your issue to occur? {
title: "Stay on stable rootless when no stable later releases are available",
current: "moby3/buildkit:v0.12.2-rootless",
want: "moby3/buildkit:v0.12.2-rootless",
tags: []string{"v0.11.2-rootless", "v0.12.2-rootless", "v0.12.2", "v0.13.0-beta1-rootless"},
},Also for the prometheus issue, I can see the image picked the correct tag in one scenario but picked the wrong one in another. Can you kindly provide me with more context into what worked and what didn't? |
Hi @nitishkumar71, Sorry, it took me weyy too long to get to this. I think I might need a bit more information on your issue. I wrote a test case for the buildkit problem and my test passed. Does this test case reflect the conditions for your issue to occur? {
title: "Stay on stable rootless when no stable later releases are available",
current: "moby3/buildkit:v0.12.2-rootless",
want: "moby3/buildkit:v0.12.2-rootless",
tags: []string{"v0.11.2-rootless", "v0.12.2-rootless", "v0.12.2", "v0.13.0-beta1-rootless"},
},Also for the prometheus issue, I can see the image picked the correct tag in one scenario but picked the wrong one in another. Can you kindly provide me with more context into what worked and what didn't? |
|
@bxffour No Worries. I again tried to test against those 2 files, as there are new tags available for them in dockerhub. They are not representing the same behavior as earlier. Theoretically your test should reproduce the behavior but it's not doing the same. I am not very sure, why. Possibly, need to look more at it. |
Description
Fix incorrect tag selection in arkade chart upgrade
Motivation and Context
design/approvedby a maintainer (required)This PR addresses an issue in the upgrade command where incorrect transitions occur between stable and RC versions. It resolves the problem encountered when two available tags are present.
For instance, when upgrading
prom/prometheus, the command mistakenly replaces the stable versionv2.44.0with the latest unstable versionv2.45.0-rc.0Similar issues arise with the
moby/buildkitimage, where the command replaces the specified versionv0.11.6-rootlesswith an unintended alternativev0.11.6.Another issue arises when two images share substrings. Specifically, when the values file contains images with names like
prom/prometheus:1.0.0andprom/prometheus:1.0.0-rc, an unintended behavior is observed when the tool attempts to update the non-rc version. The tool mistakenly matches the rc version and erroneously modifies it. The bug seems to manifest randomly, causing inconsistent behavior.This PR provides a fix to ensure accurate and expected behavior during upgrades, preventing undesired transitions between stable and RC versions.
Example
values.yml:
Command:
Output
Before:
After:
How Has This Been Tested?
I wrote a unit test to verify the modified tag selection logic
Test Cases:
Output:
I have also added a test to ensure the correct behavior of the
ReplaceValuesInHelmValuesFilefunction.Test Cases:
Output:
Types of changes
Documentation
./arkade get --format markdown./arkade install --helpChecklist:
git commit -s