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

Reduce complexity by merging configmap/secret into secret #1179

Merged

consideRatio merged 3 commits into jupyterhub:master from consideRatio:pr/merge-config-sources

Apr 23, 2022

Member

consideRatio commented Oct 29, 2020 •

edited

Loading

This PR is meant to close #1176.

The value of doing this is simply to reduce the complexity of things. As one can see from the deleted code in the Helm templates, it is quite messy code that is hard to parse and generally hard to maintain. I really hope to make this Helm chart easier to maintain by reducing complexity wherever we can.

Review notes

This is ready for merge in my mind, but it would be good to eyeball it on mybinder.org's staging environment as well.

consideRatio commented

View reviewed changes

helm-chart/binderhub/templates/secret.yaml Show resolved Hide resolved

consideRatio commented

View reviewed changes

helm-chart/binderhub/templates/deployment.yaml Show resolved Hide resolved

consideRatio commented

View reviewed changes

helm-chart/binderhub/templates/deployment.yaml Show resolved Hide resolved

consideRatio commented

View reviewed changes

helm-chart/binderhub/files/binderhub_config.py Outdated Show resolved Hide resolved

consideRatio commented

View reviewed changes

helm-chart/binderhub/files/binderhub_config.py Outdated Show resolved Hide resolved

consideRatio added the maintenance label

consideRatio requested review from minrk, betatim and sgibson91

October 29, 2020 20:47

consideRatio force-pushed the pr/merge-config-sources branch from dc33d83 to 6ca4dc0 Compare

October 30, 2020 16:36

betatim reviewed

View reviewed changes

helm-chart/binderhub/templates/deployment.yaml

@@ @@ -30,9 +30,7 @@ spec: @@
                       {{- end }}
                       {{- end }}
                     annotations:
-                      # This lets us autorestart when the configmap changes!

Member

betatim Nov 3, 2020

Can we keep this comment?

Member

manics Apr 23, 2022

Looks like this comment is still missing.

Member

betatim commented Nov 3, 2020

Are there any steps people need to take to transition from the CM+Secret setup to the Secret only setup? I think it is all automated.

Member Author

consideRatio commented Nov 3, 2020 •

edited

Loading

Thank you for the review @betatim! ❤️ 🎉

Are there any steps people need to take to transition from the CM+Secret setup to the Secret only setup? I think it is all automated.

It is all automated since Helm is always creating/updating these resources. It could make sense to have a k8s upgrade step like we have in z2jh to verify upgrades between changes work btw. I created #1195 to represent that wish.

consideRatio force-pushed the pr/merge-config-sources branch from 3f5896c to c99b31c Compare

November 3, 2020 23:31

Member Author

consideRatio commented Nov 4, 2020 •

edited

Loading

The test failure can be ignored, a rebase didn't make it run with updated tests apparently, but the error is fixed in #1201. Woops, they are fixed by #1203.

consideRatio force-pushed the pr/merge-config-sources branch from c99b31c to fc61cfb Compare

November 4, 2020 01:09

consideRatio changed the title ~~Reduce complexity by merging configmap/secret into secret~~ [MRG] Reduce complexity by merging configmap/secret into secret

minrk reviewed

View reviewed changes

helm-chart/binderhub/templates/secret.yaml Outdated Show resolved Hide resolved

consideRatio force-pushed the pr/merge-config-sources branch from fc61cfb to dc3df7f Compare

December 11, 2020 22:03

Member Author

consideRatio commented Jan 14, 2021

Hi @minrk @betatim! Just a friendly reminder of this maintenance PR.

Member Author

consideRatio commented Feb 10, 2021

@minrk does this LGTY?

consideRatio mentioned this pull request

Automatic HTTPS setup for BinderHub pangeo-data/jupyter-earth#3

Open

consideRatio added the code:helm-chart label

sgibson91 approved these changes

View reviewed changes

Member

sgibson91 left a comment

I think this is good to merge once the conflicts have been resolved?

Member

manics commented Feb 17, 2022

Ping @consideRatio! @sgibson91 has approved this subject to conflicts.

consideRatio added 2 commits

April 22, 2022 18:08


          Reduce complexity by merging configmap/secret into secret

150b648


          Retain old behaviour to only restart on relevant config changes

9968e23

consideRatio force-pushed the pr/merge-config-sources branch from dc3df7f to 9968e23 Compare

April 22, 2022 16:10

consideRatio changed the title ~~[MRG] Reduce complexity by merging configmap/secret into secret~~ Reduce complexity by merging configmap/secret into secret

Member Author

consideRatio commented Apr 22, 2022

Rebased, I think this is ready for review again!

consideRatio commented

View reviewed changes

helm-chart/binderhub/templates/deployment.yaml Show resolved Hide resolved


          Update helm-chart/binderhub/templates/deployment.yaml

51b7354

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>

consideRatio merged commit 66ca304 into jupyterhub:master

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request


          [binderhub] Automatic update for commit 0.2.0-n914.h51b7354

c5c20bd

jupyterhub/binderhub#1179 Merge pull request #1179 from consideRatio/pr/merge-config-sources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code:helm-chart maintenance