Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/*] Create and document charts policy for breaking change upgrades #5657

Closed
2 tasks done
scottrigby opened this issue May 19, 2018 · 16 comments · Fixed by #7785, #7813, #7814, #7816 or #7817
Closed
2 tasks done

[stable/*] Create and document charts policy for breaking change upgrades #5657

scottrigby opened this issue May 19, 2018 · 16 comments · Fixed by #7785, #7813, #7814, #7816 or #7817
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@scottrigby
Copy link
Member

scottrigby commented May 19, 2018

Current status

Any breaking (backwards incompatible) changes to a chart should:

  1. Bump the MAJOR version (following semver)
  2. In the README, under a section called "Upgrading", describe the manual steps necessary to upgrade to the new (specified) MAJOR version
  • Decide on policy
  • Document policy
Original summary

Hi everyone,

After @unguiculus reached started a Slack Charts maintainers DM about #5576, an unresolved problem emerged. We don't have a kubernetes/charts policy for how to handle breaking change upgrades.

Considerations

That PR made it clear we have two main choices:

  1. Add logic to the templates for every possible upgrade step (from every past to new version of an app).
    • 🚫 This was deemed to be not only an inelegant way to handle this problem on the whole, but also would make charts more difficult to maintain in the future.
  2. Follow semver to denote breaking changes (bump MAJOR version, even if the upstream project's appVersion itself did not), and somehow clearly communicate the required upgrade steps to chart users who are upgrading from relevant previous versions to the current one (the steps themselves will vary by application and old/new version).
    • ✅ This is the preferred option.

Next steps

We plan to meet about determining some helpful guidelines about this. Possibilities discussed were UPGRADE_NOTES/CHANGELOG files for charts, or adding a new section to the README (perhaps under new section header such as Upgrading, just above Installing the chart so users are more likely to see it?).

The ultimate goal of this issue is to track how we decide upon and document a consistent strategy for communicating additional info about breaking changes to users who want to upgrade from past versions of a chart.

@scottrigby
Copy link
Member Author

@unguiculus @viglesiasce @prydonius @mattfarina @lachie83 please let me know if I missed anything.

@stale
Copy link

stale bot commented Aug 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 19, 2018
@stale
Copy link

stale bot commented Sep 2, 2018

This issue is being automatically closed due to inactivity.

@stale stale bot closed this as completed Sep 2, 2018
@scottrigby scottrigby reopened this Sep 10, 2018
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 10, 2018
@scottrigby
Copy link
Member Author

scottrigby commented Sep 10, 2018

We should keep this issue open till we document this.

@desaintmartin
Copy link
Collaborator

Here is an issue where several charts are affected and need breaking changes: #7680

@juan131
Copy link
Collaborator

juan131 commented Sep 17, 2018

Hi @scottrigby

I agree with adding the instructions on a new Upgrading section. We can use sth like:

## Upgrading

### To Y.Y.Y

Backwards compatibility is not guaranteed unless you modify the labels used on the chart's deployments.
Use the workaround below to upgrade from versions previous to Y.Y.Y:

<Workaround here>

@javsalgar
Copy link
Collaborator

I tried this workaround before upgrading and fixed it:

kubectl patch deployments <deployment-name> --type=json -p='[{"op": "remove", "path": "/spec/selector/matchLabels/chart"}]'

@desaintmartin
Copy link
Collaborator

desaintmartin commented Sep 18, 2018

I tried this workaround before upgrading and fixed it:

kubectl patch deployments <deployment-name> --type=json -p='[{"op": "remove", "path": "/spec/selector/matchLabels/chart"}]'

I've just tried it on a apps/v1beta2 Statefulset (rabbitmq), and the outcome is still the same:

The StatefulSet "my-rabbitmq" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden.

Was your Deployment using >=v1beta2 as well?

For >=apps/v1beta2, the only way I've found is to:
k delete statefulsets.apps --cascade=false my-rabbitmq
Then upgrade.

@javsalgar
Copy link
Collaborator

javsalgar commented Sep 18, 2018

After checking, I confirm that the way @desaintmartin proposed is the one that works for StatefulSets.

@desaintmartin
Copy link
Collaborator

desaintmartin commented Sep 18, 2018

And after checking, I confirm that the "kubectl patch" command line does NOT work either on Deployment apps/v1beta2 and later

@desaintmartin
Copy link
Collaborator

I've tried to do a PR for redis as an example, with its own "how to upgrade" in its readme.
#7686

@javsalgar
Copy link
Collaborator

javsalgar commented Sep 19, 2018

In these charts where deployments <= apps/v1beta2, the patch command works so the guide written in #7813 and #7785 are applicable

@stale
Copy link

stale bot commented Nov 1, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 1, 2018
@stale
Copy link

stale bot commented Nov 15, 2018

This issue is being automatically closed due to inactivity.

@stale stale bot closed this as completed Nov 15, 2018
@scottrigby scottrigby added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 21, 2018
@scottrigby scottrigby reopened this Nov 21, 2018
@scottrigby
Copy link
Member Author

I should close this - we've adopted the README policy. I'll update the summary above.

@scottrigby
Copy link
Member Author

scottrigby commented Feb 6, 2019

Eh, review guidelines still need to be updated

@scottrigby scottrigby reopened this Feb 6, 2019
scottrigby added a commit that referenced this issue Feb 6, 2019
Fixes #5657 

Signed-off-by: Scott Rigby <scott@r6by.com>
tbuchier pushed a commit to tbuchier/charts that referenced this issue Feb 14, 2019
Fixes helm#5657 

Signed-off-by: Scott Rigby <scott@r6by.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.