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.

Sign up for GitHub

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

Jump to bottom

helm upgrade uses reuse-values by default #525

Closed
joejulian opened this issue Feb 10, 2023 · 10 comments · May be fixed by #652
Closed

helm upgrade uses reuse-values by default #525

joejulian opened this issue Feb 10, 2023 · 10 comments · May be fixed by #652
Labels
Stale

Comments

@joejulian
Copy link
Contributor

Is this a request for help?:

Is this a BUG REPORT or FEATURE REQUEST? (choose one):

BUG REPORT

Version of Helm and Kubernetes:

version.BuildInfo{Version:"v3.11.0", GitCommit:"472c5736ab01133de504a826bd9ee12cbe4e7904", GitTreeState:"clean", GoVersion:"go1.19.5"}

ct version 2.7.1

What happened:
Upgrade failed when adding a new subchart in the new version because the new default chart values are ignored when reuse-values is specified. This caused the overrides in the new parent chart's values to be omitted and the subcharts defaults were used instead. As those overrides are required for the subchart to be a subchart, this caused the upgrade to fail.

When testing without reuse-values the upgrade is performed correctly.

Since using that flag is not a commonly documented way of upgrading charts, it should not be the default.

What you expected to happen:

I expected the common helm upgrade <release> <chart> --wait to be executed.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know:
@xDmitriev
Copy link

xDmitriev commented Feb 10, 2023 •
edited
Loading

I faced the exactly same issue. When I added new items in values.yaml, ct install --upgrade is failed all the time since it does not respect new values

joejulian added a commit to joejulian/helm-charts that referenced this issue Feb 11, 2023
@joejulian
use my fork until we can get changes upstream
f4c967a 
use my fork for chart-testing until a change can be merged upstream to
not use the `--reuse-values` flag

helm/chart-testing#525
alejandroEsc added a commit to redpanda-data/helm-charts that referenced this issue Feb 13, 2023
@joejulian @alejandroEsc
redpanda: add console as subchart to redpanda (#275)
1b7f820 
* redpanda: add console as subchart to redpanda

* use my fork until we can get changes upstream

use my fork for chart-testing until a change can be merged upstream to
not use the `--reuse-values` flag

helm/chart-testing#525

* allow postStart first-sasl-user hack to run even with only one replica

* fix: type

* Delete admin.conf

---------

Co-authored-by: alejandroesc <[email protected]>
@xDmitriev
Copy link

Guys, what is the historical reason for having --reuse-values hardcoded here?

chart-testing/pkg/tool/helm.go

Line 74 in b8ad35c

"--reuse-values", "--wait", h.extraArgs, h.extraSetArgs); err != nil {

@xDmitriev
Copy link

@joejulian Does the upgrade mode work in your case, if you remove --reuse-values flag? Asking because it does not work in my case, I tried my own and your images, and there is a timed out waiting for the condition during the chart upgrade. But in fact, all pods getting ready after the upgrade, for some reason helm does not see that.

@joejulian
Copy link
Contributor Author

@xDmitriev Yes, it works great.

@xDmitriev
Copy link

@xDmitriev Yes, it works great.

Could you please share your CLI arguments? Or is it just ct install --upgrade ?

@joejulian
Copy link
Contributor Author

https://github.com/redpanda-data/helm-charts/blob/main/.github/workflows/pull_requests.yaml#L89

The config file is:
https://github.com/redpanda-data/helm-charts/blob/main/.github/ct.yaml

@joejulian joejulian mentioned this issue Mar 11, 2023
Closed
This was referenced Aug 16, 2023
Merged
Open
@ebaron ebaron mentioned this issue Aug 22, 2023
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Nov 3, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 8, 2023

This issue was closed because it has been stalled for 5 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2023
@ppawlowski ppawlowski mentioned this issue Feb 28, 2024
Merged
11 tasks
@tthvo
Copy link

tthvo commented May 13, 2024

I think it is still useful to have and considering helm/helm#9653, we can also add support for --reset-then-reuse-values. Any on-going work to continue #531 @joejulian ?

I can try (relatively new here) and help take a look into it if needed :D

This was referenced May 21, 2024
Merged
Open
@jochbru
Copy link

jochbru commented Jul 12, 2024

This issue should not be closed IMHO, the hard coding of the reuse values makes the upgrade testing workflow essentially use when trying to test helm charts. Because adding new default values in the Helm chart is fine but not support by this testing tool at the moment.

@jochbru jochbru mentioned this issue Jul 12, 2024
Merged
@bobertrublik bobertrublik mentioned this issue Oct 1, 2024
Merged
@mhrabovcin mhrabovcin mentioned this issue Nov 11, 2024
Merged
@ppawlowski ppawlowski mentioned this issue Nov 13, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
4 participants
@joejulian @jochbru @xDmitriev @tthvo