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

fix incorrect tag selection in arkade chart upgrade #940

Closed
wants to merge 1 commit into from

Conversation

bxffour
Copy link
Contributor

@bxffour bxffour commented Jun 20, 2023

Description

Fix incorrect tag selection in arkade chart upgrade

Motivation and Context

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 version v2.44.0 with the latest unstable version v2.45.0-rc.0

Similar issues arise with the moby/buildkit image, where the command replaces the specified version v0.11.6-rootless with an unintended alternative v0.11.6.

Another issue arises when two images share substrings. Specifically, when the values file contains images with names like prom/prometheus:1.0.0 and prom/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:

builder:
  image: moby/buildkit:v0.11.5-rootless
prom:
  image: prom/prometheus:v2.43.0
prom_rc:
  image: prom/prometheus:v2.43.0-rc.0

Command:

arcade chart upgrade -f values.yml

Output

Before:

builder:
  image: moby/buildkit:v0.11.6
prom:
  image: prom/prometheus:v2.45.0-rc.0
prom_rc:
# this is happens because the image was inadvertently modified
# when 'prom' was being modified.
  image: prom/prometheus:v2.45.0-rc.0-rc.0

After:

builder:
  image: moby/buildkit:v0.11.6-rootless
prom:
  image: prom/prometheus:v2.44.0
prom_rc:
# rcs are upgraded to the latest stable release when available
  image: prom/prometheus:v2.44.0

How Has This Been Tested?

I wrote a unit test to verify the modified tag selection logic

Test Cases:

	tests := []test{
		{
			title:   "Promote from a rootless build to the latest rootless build",
			current: "moby/buildkit:v0.11.5-rootless",
			want:    "moby/buildkit:v0.11.6-rootless",
			tags:    []string{"0.10.6-rootless", "v0.11.5", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"},
		},
		{
			title:   "Promote from a rootless build to the latest rootless build",
			current: "moby1/buildkit:v0.10.0-rootless",
			want:    "moby1/buildkit:v0.11.6-rootless",
			tags:    []string{"0.10.0", "0.10.6", "0.10.6-rootless", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"},
		},
		{
			title:   "Promote from non-rootless build to the latest stable build",
			current: "moby2/buildkit:v0.8.0",
			want:    "moby2/buildkit:v0.11.6",
			tags:    []string{"v0.8.0", "0.10.6-rootless", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"},
		},
		{
			title:   "Promote from stable release to the latest stable release",
			current: "prom/prometheus:v2.42.0",
			want:    "prom/prometheus:v2.44.0",
			tags:    []string{"2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"},
		},
		{
			title:   "Promote from stable release to the latest stable release",
			current: "prom1/prometheus:v2.2.0",
			want:    "prom1/prometheus:v2.44.0",
			tags:    []string{"v2.1.0", "v2.2.0", "2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"},
		},
		{
			title:   "Promote from release condidate to the latest stable release",
			current: "bitnami/postgresql:v2.42.0-rc.2",
			want:    "bitnami/postgresql:v2.44.0",
			tags:    []string{"v2.42.0-rc.5", "2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"},
		},
		{
			title:   "Promote from prerelease version to the latest prerelease version",
			current: "bitnami/rabbitmq:v2.22.0-rc1",
			want:    "bitnami/rabbitmq:v2.22.0-rc3",
			tags:    []string{"v2.21.0", "v2.22.0-rc1", "v2.22.0-rc2", "v2.22.0-rc3"},
		},
		{
			title:   "Remain on current version when there isn't a later version",
			current: "openfaas/openfaas:v2.23.3",
			want:    "openfaas/openfaas:v2.23.3",
			tags:    []string{"v2.23.3", "v2.21.0", "v2.15.6"},
		},
	}

Output:

=== RUN   Test_ChartUpgrade
2023/07/08 20:25:12 Wrote 7 updates to: /tmp/arkade_3569690160.yml
=== RUN   Test_ChartUpgrade/Promote_from_a_rootless_build_to_the_latest_rootless_build
=== RUN   Test_ChartUpgrade/Promote_from_a_rootless_build_to_the_latest_rootless_build#01
=== RUN   Test_ChartUpgrade/Promote_from_non-rootless_build_to_the_latest_stable_build
=== RUN   Test_ChartUpgrade/Promote_from_stable_release_to_the_latest_stable_release
=== RUN   Test_ChartUpgrade/Promote_from_stable_release_to_the_latest_stable_release#01
=== RUN   Test_ChartUpgrade/Promote_from_release_condidate_to_the_latest_stable_release
=== RUN   Test_ChartUpgrade/Promote_from_prerelease_version_to_the_latest_prerelease_version
=== RUN   Test_ChartUpgrade/Remain_on_current_version_when_there_isn't_a_later_version
--- PASS: Test_ChartUpgrade (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_a_rootless_build_to_the_latest_rootless_build (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_a_rootless_build_to_the_latest_rootless_build#01 (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_non-rootless_build_to_the_latest_stable_build (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_stable_release_to_the_latest_stable_release (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_stable_release_to_the_latest_stable_release#01 (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_release_condidate_to_the_latest_stable_release (0.00s)
    --- PASS: Test_ChartUpgrade/Promote_from_prerelease_version_to_the_latest_prerelease_version (0.00s)
    --- PASS: Test_ChartUpgrade/Remain_on_current_version_when_there_isn't_a_later_version (0.00s)
PASS
ok      command-line-arguments  0.007s

I have also added a test to ensure the correct behavior of the ReplaceValuesInHelmValuesFile function.

Test Cases:

		tests := []struct {
			current string
			update  string
		}{
			{current: "ghcr.io/openfaas/faas-netes:0.1.0", update: "ghcr.io/openfaas/faas-netes:0.2.0"},
			{current: "ghcr.io/openfaas/faas-netes:0.1.0-rc", update: "ghcr.io/openfaas/faas-netes:0.2.0"},
			{current: "prom/prometheus:v2.43.0", update: "prom/prometheus:v2.45.0"},
			{current: "prom/prometheus:v2.43.0-rc.0", update: "prom/prometheus:v2.45.0"},
			{current: "ghcr.io/openfaas/faas-netes:0.1.0-rc.1", update: "ghcr.io/openfaas/faas-netes:0.2.0"},
			{current: "ghcr.io/openfaas/faas-netes:0.1.0-rc.1-rc.2", update: "ghcr.io/openfaas/faas-netes:0.2.0"},
		}

Output:

=== RUN   Test_ReplaceValuesInHelmValuesFile
--- PASS: Test_ReplaceValuesInHelmValuesFile (0.01s)
PASS
ok      command-line-arguments  0.010s

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Documentation

  • I have updated the list of tools in README.md if (required) with ./arkade get --format markdown
  • I have updated the list of apps in README.md if (required) with ./arkade install --help

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have tested this on arm, or have added code to prevent deployment

@bxffour bxffour closed this Jun 23, 2023
@bxffour bxffour reopened this Jun 23, 2023
@alexellis
Copy link
Owner

Could you also show that upgrade still works?

Try these as examples:

builder:
  image: moby/buildkit:v0.8.0
prom:
  image: prom/prometheus:v2.2.0

And:

builder:
  image: moby/buildkit:v0.10.0-rootless

This one is going to be more complex:

prom:
  image: prom/prometheus:v2.42.0-rc.0

Technically, it should increase to a stable version.

So when moving from an rc suffix to a stable suffix, that that is newer.

@bxffour
Copy link
Contributor Author

bxffour commented Jun 23, 2023

Could you also show that upgrade still works?

Try these as examples:

builder:
  image: moby/buildkit:v0.8.0
prom:
  image: prom/prometheus:v2.2.0

And:

builder:
  image: moby/buildkit:v0.10.0-rootless

This one is going to be more complex:

prom:
  image: prom/prometheus:v2.42.0-rc.0

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

@alexellis
Copy link
Owner

Thanks 👍

}
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a comment here?

}

latest := vv[0]
if latest.Prerelease() != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you extract to a small function with a self-describing name and a comment, or add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

func Test_ChartUpgrade(t *testing.T) {
tests := []test{
{
name: "builder_v11_rootless",
Copy link
Owner

Choose a reason for hiding this comment

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

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.

expected: "moby/buildkit:v0.11.6",
},
{
name: "prom_Stable",
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

This is a really good start.

But we need to control the data rather than calling out to the Internet in the unit tests, otherwise these may not always pass or give the expected results

@alexellis
Copy link
Owner

Hi @bxffour what do we have in the latest changes?

Is there anything to update in the PR description too?

Thanks

@bxffour
Copy link
Contributor Author

bxffour commented Jul 7, 2023

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:

  1. I have refactored the tag selection logic by isolating it through an interface, which allowed me to effectively mock the tag lister and gain better control over the test data. This approach proved beneficial as it helped in handling network calls and improved the overall testability of the code. Furthermore, I have made modifications to the tests, utilizing a predefined list of tags, resulting in enhanced predictability and control of the test scenarios. I have also removed the condition in the Makefile that excluded the chart upgrade test, as it is no longer necessary due to these changes.

  2. I have modified the helm.ReplaceValuesInHelmValuesFile function to evaluate replacements line by line instead of using strings.ReplaceAll. This change addresses a previous issue where partial matches of image names could lead to unexpected behavior, ensuring more accurate and reliable replacements.

@bxffour bxffour force-pushed the fix/issue-936 branch 2 times, most recently from 0ef7287 to cfcc607 Compare July 8, 2023 20:38
@alexellis
Copy link
Owner

@welteki could you take a look at the PR and review please?

@alexellis
Copy link
Owner

@welteki thanks for the review

@bxffour I can still see two of my comments haven't been acknowledged or addressed, please can you take another look at these?

One was about extracting a method, the other was about adding a comment to explain something. Then we'll get this merged

@alexellis
Copy link
Owner

I have modified the helm.ReplaceValuesInHelmValuesFile function to evaluate replacements line by line instead of using strings.ReplaceAll. This change addresses a previous issue where partial matches of image names could lead to unexpected behavior, ensuring more accurate and reliable replacements.

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.

@bxffour

@bxffour
Copy link
Contributor Author

bxffour commented Jul 25, 2023

I have modified the helm.ReplaceValuesInHelmValuesFile function to evaluate replacements line by line instead of using strings.ReplaceAll. This change addresses a previous issue where partial matches of image names could lead to unexpected behavior, ensuring more accurate and reliable replacements.

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.

@bxffour

No there isn't. I'd work on adding one asap

@bxffour bxffour reopened this Jul 30, 2023
Copy link
Contributor

@nitishkumar71 nitishkumar71 left a comment

Choose a reason for hiding this comment

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

I tried to test these on charts for faas-netes and faasd. There are two cases, where possibly update did not work as expected.

  1. Prometheus image got updated from v2.47.0 to v2.48.0-rc.0 i.e stable to rc. Also when we tried to upgrade image into faasd, prometheus image was updated from v2.46.0 to v.2.47.0 i.e. stable to stable.
  2. Buildkit image got updated from v0.12.2-rootless to v0.13.0-beta1-rootless i.e. stable to beta.

@bxffour
Copy link
Contributor Author

bxffour commented Oct 21, 2023

I tried to test these on charts for faas-netes and faasd. There are two cases, where possibly update did not work as expected.

  1. Prometheus image got updated from v2.47.0 to v2.48.0-rc.0 i.e stable to rc. Also when we tried to upgrade image into faasd, prometheus image was updated from v2.46.0 to v.2.47.0 i.e. stable to stable.
  2. Buildkit image got updated from v0.12.2-rootless to v0.13.0-beta1-rootless i.e. stable to beta.

Thanks for your feedback. I'd replicate the problem and submit an updated PR asap.

@bxffour
Copy link
Contributor Author

bxffour commented Dec 11, 2023

I tried to test these on charts for faas-netes and faasd. There are two cases, where possibly update did not work as expected.

  1. Prometheus image got updated from v2.47.0 to v2.48.0-rc.0 i.e stable to rc. Also when we tried to upgrade image into faasd, prometheus image was updated from v2.46.0 to v.2.47.0 i.e. stable to stable.
  2. Buildkit image got updated from v0.12.2-rootless to v0.13.0-beta1-rootless i.e. stable to beta.

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?

@bxffour
Copy link
Contributor Author

bxffour commented Dec 11, 2023

I tried to test these on charts for faas-netes and faasd. There are two cases, where possibly update did not work as expected.

  1. Prometheus image got updated from v2.47.0 to v2.48.0-rc.0 i.e stable to rc. Also when we tried to upgrade image into faasd, prometheus image was updated from v2.46.0 to v.2.47.0 i.e. stable to stable.
  2. Buildkit image got updated from v0.12.2-rootless to v0.13.0-beta1-rootless i.e. stable to beta.

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?

@nitishkumar71
Copy link
Contributor

@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.

@rgee0
Copy link
Contributor

rgee0 commented Sep 4, 2024

/close: significant passage of time and possibly superseded by #1087 and #1110.

@derek derek bot closed this Sep 4, 2024
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.

Edge-cases where arkade chart upgrade picks the wrong tag
5 participants