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

[stable/redis] Major version bump: Fix chart not being upgradable #7686

Merged
merged 1 commit into from
Sep 24, 2018
Merged

[stable/redis] Major version bump: Fix chart not being upgradable #7686

merged 1 commit into from
Sep 24, 2018

Conversation

desaintmartin
Copy link
Collaborator

@desaintmartin desaintmartin commented Sep 12, 2018 •
edited
Loading

What this PR does / why we need it:

Major version bump: Fix chart not being upgradable by removing 'chart' label from spec.selector or spec.VolumeClaimTemplate.

Since this is a breaking change, take this opportunity to also update labels to match the recommendations from Helm: https://github.com/helm/helm/blob/master/docs/chart_best_practices/labels.md

Also set a selector to all Deployments.

Special notes for your reviewer:

See #7680 and #7803 for explanations.

See #7692 for ongoing work to update Review Guidelines.

cc @prydonius @tompizmor @sameersbn @carrodher @juan131

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2018
@ey-bot ey-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Sep 12, 2018
@juan131
Copy link
Collaborator

juan131 commented Sep 17, 2018

We'll also need to update the README with any manual steps to upgrade to this breaking change. See #5657

@desaintmartin
Copy link
Collaborator Author

/do-not-merge
Until the procedure is decided and documented at #5657
However, I still appreciate other feedbacks! :)

@desaintmartin
Copy link
Collaborator Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2018
jlegrone
jlegrone reviewed Sep 17, 2018
View reviewed changes
Copy link
Member

@jlegrone jlegrone left a comment

Choose a reason for hiding this comment

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

@desaintmartin as part of this breaking change it might make sense to update the existing labels to be aligned with the current helm label conventions:

https://docs.helm.sh/chart_best_practices/#standard-labels

release --> app.kubernetes.io/instance
chart --> helm.sh/chart
role --> app.kubernetes.io/component
app --> app.kubernetes.io/name
heritage --> app.kubernetes.io/managed-by

@desaintmartin desaintmartin mentioned this pull request Sep 18, 2018
Merged
@ey-bot ey-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Sep 18, 2018
@desaintmartin
Copy link
Collaborator Author

I've added a small "how to upgrade" part in the Readme as a proposition to be discussed.

@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 18, 2018
@desaintmartin
Copy link
Collaborator Author

Moved all labels to new recommendations.

I think we have all we need now. Feel free to review.

@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 18, 2018
desaintmartin
desaintmartin commented Sep 18, 2018
View reviewed changes
stable/redis/README.md Outdated Show resolved Hide resolved
@desaintmartin desaintmartin mentioned this pull request Sep 18, 2018
Closed
2 tasks
@ey-bot ey-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Sep 23, 2018
@desaintmartin
[stable/redis] Major version bump: Fix chart not being upgradable by …
f20a0bf 
…removing 'chart' label from spec.selector or spec.VolumeClaimTemplate.

Also set a selector to all Deployments.

See #7680.

Signed-off-by: Cédric de Saint Martin <[email protected]>
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 23, 2018
@desaintmartin
Copy link
Collaborator Author

desaintmartin commented Sep 23, 2018 •
edited
Loading

I think it is safer to separate the two changes : 1/ fixing the upgrade and 2/ applying new labels convention, like it has been done for all other PRs.

I think we can remove do-not-merge. cc @juan131

@juan131
Copy link
Collaborator

juan131 commented Sep 24, 2018

Totally agree @desaintmartin !! Thanks for updating the README.md

@juan131
Copy link
Collaborator

juan131 commented Sep 24, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: desaintmartin, juan131

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2018
@juan131
Copy link
Collaborator

juan131 commented Sep 24, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 24, 2018
@desaintmartin
Copy link
Collaborator Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2018
@k8s-ci-robot k8s-ci-robot merged commit c3bbcad into helm:master Sep 24, 2018
This was referenced Sep 25, 2018
Closed
Closed
Merged
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
@desaintmartin @jicowan
[stable/redis] Major version bump: Fix chart not being upgradable by …
2fe27d8 
…removing 'chart' label from spec.selector or spec.VolumeClaimTemplate. (helm#7686)

Also set a selector to all Deployments.

See helm#7680.

Signed-off-by: Cédric de Saint Martin <[email protected]>
Signed-off-by: jenkin-x <[email protected]>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
@desaintmartin @Jnig
[stable/redis] Major version bump: Fix chart not being upgradable by …
1164d38 
…removing 'chart' label from spec.selector or spec.VolumeClaimTemplate. (helm#7686)

Also set a selector to all Deployments.

See helm#7680.

Signed-off-by: Cédric de Saint Martin <[email protected]>
Signed-off-by: Jakob Niggel <[email protected]>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
@desaintmartin @wgiddens
[stable/redis] Major version bump: Fix chart not being upgradable by …
54d6f9c 
…removing 'chart' label from spec.selector or spec.VolumeClaimTemplate. (helm#7686)

Also set a selector to all Deployments.

See helm#7680.

Signed-off-by: Cédric de Saint Martin <[email protected]>
@thomas-riccardi thomas-riccardi mentioned this pull request Apr 17, 2019
Closed
@desaintmartin desaintmartin deleted the redis-upgrade branch November 21, 2019 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jlegrone jlegrone jlegrone left review comments

@scottrigby scottrigby scottrigby left review comments

@prydonius prydonius Awaiting requested review from prydonius

@tompizmor tompizmor Awaiting requested review from tompizmor

Assignees

@juan131 juan131

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@desaintmartin @juan131 @k8s-ci-robot @scottrigby @jlegrone @ey-bot