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: it should upgrade to new version instead of the old one #434

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

paologallinaharbur
Copy link
Contributor

What this PR does / why we need it:
When testing the upgrade we should actually install the new version

Old behaviour (installing the same version, not catching the issue with selectors)

Testing upgrades of chart 'nri-prometheus => (version: "2.1.2", path: "charts/nri-prometheus")' relative to previous revision 'nri-prometheus => (version: "2.1.1", path: "ct_previous_revision2130172757/charts/nri-prometheus")'...
[...]
>>> helm install nri-prometheus-y2dumyevqw ct_previous_revision2130172757/charts/nri-prometheus --namespace ct --wait --values ct_previous_revision2130172757/charts/nri-prometheus/ci/test-lowdatamode-values.yaml
>>> helm upgrade nri-prometheus-y2dumyevqw ct_previous_revision2130172757/charts/nri-prometheus --namespace ct --reuse-values --wait

New Behaviour (installing the new version, catching the issue)

Testing upgrades of chart 'nri-prometheus => (version: "2.1.2", path: "charts/nri-prometheus")' relative to previous revision 'nri-prometheus => (version: "2.1.1", path: "ct_previous_revision78831704/charts/nri-prometheus")'...
[...]
>>> helm install nri-prometheus-avhvhn5q0a ct_previous_revision78831704/charts/nri-prometheus --namespace ct --wait --values ct_previous_revision78831704/charts/nri-prometheus/ci/test-lowdatamode-values.yaml
>>> helm upgrade nri-prometheus-avhvhn5q0a charts/nri-prometheus --namespace ct --reuse-values --wait
Error: UPGRADE FAILED: cannot patch "nri-prometheus" with kind Deployment: Deployment.apps "nri-prometheus" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/name":"nri-prometheus"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Which issue this PR fixes

Fix #433

Special notes for your reviewer:

Please let me know if I am misunderstanding the feature or the code (it is a vely likely possibility 😄 )

@paologallinaharbur
Copy link
Contributor Author

If interested, it would make sense as well to add some tests to cover the bug (if that's a bug 😅 ). For example with two charts having as only difference the selectors, like:

		{
			"change immutable deployment.spec.selector field in new version of the chart,
			"test_charts/simple-deployment",
			"test_charts/simple-deployment-different-selector",
			processError,
		},

Moreover, it would make sense to garbage collect the upgrade test change immutable deployment.spec.selector field since it covers helm/charts#7726 that it is due to apps/v1beta1, but it using to do so a v1 chart, .

@paologallinaharbur
Copy link
Contributor Author

Hello @cpanato, 😄
do you know who can we ask to in order to know if it is a bug of the --upgrade flag or not?
I had the feeling that at least in our use case it is a possible source of false positives

@paologallinaharbur paologallinaharbur changed the title feat(upgrade): it should upgrade to new version instead of the old one FIX: it should upgrade to new version instead of the old one May 27, 2022
@LeoColomb
Copy link
Contributor

I also believe it's a bug, and this fix seems relevant.
Should also close #198

@cpanato cpanato requested a review from davidkarlsen June 1, 2022 07:30
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

i guess this is correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade tests is installing and upgrading the same chart version
4 participants