Kubernetes recommanded labels and custom labels by nlamirault · Pull Request #25 · oauth2-proxy/manifests

nlamirault · 2021-04-21T06:50:22Z

Support for :

Kubernetes recommanded labels : See: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
custom labels to add to all resources

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

desaintmartin · 2021-04-21T07:52:44Z

Thanks for the contribution! Appart from the failed test, this is a breaking change that requires chart uninstallation (or at least k delete X --cascade=false for all controllers) that should be discussed with everybody before merging.

At least, we need to state the breaking change in the version and provide an upgrade procedure.

pierluigilenoci · 2021-07-01T08:43:27Z

@nlamirault could you please revisit your PR according to the suggestions?

pierluigilenoci · 2021-07-01T09:23:04Z

@nlamirault thanks but things are still missing. The version bump must be of the major version, a migration procedure must be included and explicitly written that it is a breaking changes.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault · 2021-07-01T12:26:30Z

@pierluigilenoci do you have an example of a chart with a migration procedure ?

pierluigilenoci · 2021-07-01T13:05:06Z

@nlamirault maybe something like that is enough https://github.com/oauth2-proxy/manifests/tree/main/helm/oauth2-proxy#to-400

For me is enough to make it clear that it is a breaking change and write the migration instructions but I leave the last word to the other maintainers who are more experienced than me.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault · 2021-07-06T21:06:32Z

@pierluigilenoci @desaintmartin i make a section for next release.

EtienneBarbier

I closed PR #43. Everything is already here and compatible with my use-case. Thank you @nlamirault ;)

pierluigilenoci · 2021-07-07T10:15:35Z

@nlamirault from the point of view of the chart everything seems fine, perhaps the migration guide could be more exhaustive because it does not list the necessary steps to migrate without problems.

pierluigilenoci · 2021-07-08T11:36:30Z

@nlamirault the PR needs a chart bump. And maybe a squash.

pierluigilenoci · 2021-07-08T13:36:57Z

@nlamirault sorry to give you bad news but the build failed.

helm/oauth2-proxy/README.md

helm/oauth2-proxy/templates/_helpers.tpl

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

pierluigilenoci · 2021-08-16T14:09:21Z

@desaintmartin could you please take a look?

desaintmartin · 2021-09-02T15:55:16Z

Last note before merging: have you actually tested that --cascade=false deletion then upgrade works as intended (kubernetes re-attaching the old pod to the new controller and deleting it)?

nlamirault · 2021-09-02T16:10:04Z

@desaintmartin No. My homelab is dead actually :(

desaintmartin · 2021-09-13T09:41:17Z

I've just tested, and actually the orphaned Pod gets lost and is not recovered/deleted by the new Deployment. I suppose the only we is to have a downtime with a Deployment deletion WITH cascade...

desaintmartin · 2021-09-13T16:47:42Z

helm/oauth2-proxy/README.md

+In order to upgrade, delete the Deployment before upgrading:
+
+```bash
+kubectl delete deployment --cascade=false my-release-oauth2-proxy


cascade will not work, unfortunately...

I find this : https://github.com/codecentric/helm-charts/blob/master/charts/keycloak/README.md#from-chart-versions--500. Could you perform another test with this script @desaintmartin ?

This should work out of the box but becomes quite complex and error-prone compared to simply delete and have a few downtime.
@pierluigilenoci what do you think?

Maybe I don't fully understand the problem, but it could be left as a backup option only for those who don't want to have downtime. I like to make everything automatic via script but this seems a bit risky to me.

What I propose:

document the kubectl delete deployment without cascade

as a fallback for user who don't want downtime: document this

@desaintmartin I totally agree

@nlamirault are you ok with the proposed solution?

@desaintmartin it seems to me that now the documentation is ok.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Allex1 · 2021-10-25T11:31:00Z

@nlamirault @pierluigilenoci should we tackle annotations as well in this pr? I've linked a pr for custom config annotations in case we should treat it separately

pierluigilenoci · 2021-10-25T12:40:01Z

@nlamirault @pierluigilenoci should we tackle annotations as well in this pr? I've linked a pr for custom config annotations in case we should treat it separately

@Allex1 it might also make sense to me but this PR is so close to closure that I wouldn't want to disturb it. 😛

Allex1 · 2021-10-25T12:43:46Z

@nlamirault @pierluigilenoci should we tackle annotations as well in this pr? I've linked a pr for custom config annotations in case we should treat it separately

@Allex1 it might also make sense to me but this PR is so close to closure that I wouldn't want to disturb it. 😛

kk, Opened #62

pierluigilenoci · 2021-10-25T13:22:12Z

@desaintmartin if you give me your final approval I merge the PR. ❤️

desaintmartin · 2021-10-25T20:19:47Z

Lgtm!

pierluigilenoci · 2021-10-26T07:07:26Z

DONE!!!! ❤️ 🚀

nlamirault added 3 commits April 21, 2021 08:49

Add: Kubernetes recommanded labels

cbd5275

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Add: documentation

6ff144f

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Fix: documentation

f080106

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault changed the title ~~Add: Kubernetes recommanded labels~~ Kubernetes recommanded labels Apr 21, 2021

Merge branch 'main' into feat/k8s-labelss

91bc682

nlamirault changed the title ~~Kubernetes recommanded labels~~ Kubernetes recommanded labels and custom labels Jun 2, 2021

todo bot mentioned this pull request Jun 5, 2021

Portefaix version label support portefaix/portefaix-kubernetes#630

Closed

Merge branch 'main' into feat/k8s-labelss

416ca99

pierluigilenoci requested review from JoelSpeed, NickMeves, desaintmartin and pierluigilenoci July 1, 2021 09:23

EtienneBarbier mentioned this pull request Jul 1, 2021

Add possibility to specify extra labels #43

Closed

nlamirault added 2 commits July 1, 2021 14:04

Update: bump major version

829492d

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Update: Split selector labels and refactoring indentation

0ea4dc4

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault added 2 commits July 6, 2021 23:01

Merge branch 'main' into feat/k8s-labelss

3b33922

Add: documentation for next release

41faf42

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

EtienneBarbier previously approved these changes Jul 7, 2021

View reviewed changes

Merge branch 'main' into feat/k8s-labelss

5933de7

-sFix: name

d159e63

nlamirault dismissed EtienneBarbier’s stale review via d159e63 July 8, 2021 14:06

JoelSpeed reviewed Jul 10, 2021

View reviewed changes

helm/oauth2-proxy/README.md Outdated Show resolved Hide resolved

Merge branch 'main' into feat/k8s-labelss

1c61c5b

EtienneBarbier reviewed Aug 12, 2021

View reviewed changes

helm/oauth2-proxy/templates/_helpers.tpl Outdated Show resolved Hide resolved

nlamirault added 2 commits August 16, 2021 14:14

Fix: remove indentation

3c164ec

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Fix: upgrade documentation

bd2ee25

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Merge branch 'main' into feat/k8s-labelss

263c8e1

desaintmartin previously approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'main' into feat/k8s-labelss

3759503

desaintmartin reviewed Sep 13, 2021

View reviewed changes

nlamirault added 2 commits October 16, 2021 18:37

Merge branch 'main' into feat/k8s-labelss

3f30079

Add: documentation

0427131

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault dismissed desaintmartin’s stale review via 0427131 October 16, 2021 16:46

pierluigilenoci requested a review from desaintmartin October 25, 2021 09:07

pierluigilenoci approved these changes Oct 25, 2021

View reviewed changes

Allex1 mentioned this pull request Oct 25, 2021

Add custom config annotations #62

Merged

desaintmartin approved these changes Oct 25, 2021

View reviewed changes

pierluigilenoci merged commit eb96b38 into oauth2-proxy:main Oct 26, 2021

Conversation

nlamirault commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

desaintmartin commented Apr 21, 2021

Uh oh!

pierluigilenoci commented Jul 1, 2021

Uh oh!

pierluigilenoci commented Jul 1, 2021

Uh oh!

nlamirault commented Jul 1, 2021

Uh oh!

pierluigilenoci commented Jul 1, 2021

Uh oh!

nlamirault commented Jul 6, 2021

Uh oh!

EtienneBarbier left a comment

Choose a reason for hiding this comment

Uh oh!

pierluigilenoci commented Jul 7, 2021

Uh oh!

pierluigilenoci commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierluigilenoci commented Jul 8, 2021

Uh oh!

Uh oh!

Uh oh!

pierluigilenoci commented Aug 16, 2021

Uh oh!

desaintmartin commented Sep 2, 2021

Uh oh!

nlamirault commented Sep 2, 2021

Uh oh!

desaintmartin commented Sep 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Allex1 commented Oct 25, 2021

Uh oh!

pierluigilenoci commented Oct 25, 2021

Uh oh!

Allex1 commented Oct 25, 2021

Uh oh!

pierluigilenoci commented Oct 25, 2021

Uh oh!

desaintmartin commented Oct 25, 2021

Uh oh!

pierluigilenoci commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nlamirault commented Apr 21, 2021 •

edited

Loading

pierluigilenoci commented Jul 8, 2021 •

edited

Loading