support extraFiles for binderhub pod #1472

minrk · 2022-04-25T10:40:24Z

imports extraFiles helpers, spec from JupyterHub, applied to the binderhub pod

Possibly more general solution to #1471

- top-level templates is dict of filename: contents, e.g. "page.html": "{% extends "templates/page.html" %}" - mount custom templates dir in /etc/binderhub/templates Existing template config already supports this, so it's just making the files available we need.

minrk · 2022-04-25T11:07:38Z

Hm, we have a problem in CI because it's now impossible to update the helm schema and pass tests, because the same config is passed both to the current and previous version of binderhub.

If the schema weren't so strict (i.e. rejecting addtionalProperties), this would be less of an issue.

choldgraf

This seems like a nice and relatively lightweight way to add this functionality without increasing our configuration surface area each time we want another page / section to be extensible...though we should probably improve our documentation around how to do this because many people may not be familiar with the template names they should use for over-riding etc...

consideRatio · 2022-04-25T12:47:30Z

This seems like [hub|singleuser].extraFiles but specialized for templates. Should we go for the even more general approach of extraFiles instead of this?

I'm probably not willing to invest my own time into this effort at the moment so please go ahead with this template specific approach if you think it makes sense. I know for z2jh it was just one use case to mount templates while mounting configuration files etc would also be relevant, due to that we developed this more general support both for the hub pod and user pods. Overall, this feature seems to have been a great success.

Z2JH configuration reference
Z2JH code implementation
- extraFiles helpers defined. These can perhaps also be reused directly from this helm chart as it depends on the jupyterhub Helm chart.
- extraFiles helpers referenced from a k8s Secret

- install an extra 'old' yaml config when installing against an old binderhub version - this file should only exist during PRs that change the schema, delete immediately after merging

minrk · 2022-04-25T13:47:27Z

This seems like [hub|singleuser].extraFiles but specialized for templates

It is! I actually used your extraFiles implementation as reference. JupyterHub is much more infinitely extensible, so there are more general files that make sense to drop-in, especially files we as chart maintainers don't need to know about. I'm not sure I see a similar arbitrary files use case in BinderHub, so I thought this would be simpler, both to implement (the whole thing is smaller than just the helpers for extraFiles) and use (deployments don't need to know where we put templates). On the other hand, I can imagine expert JupyterHub deployers (and maintainers) liking the familiarity of extraFiles consistently applied everywhere.

minrk · 2022-04-25T14:02:49Z

This now imports extraFiles exactly as-is (thanks for the links @consideRatio), so the feature is consistent across jupyterhub/binderhub.

minrk · 2022-04-25T14:04:00Z

FWIW, I couldn't figure out how to add a simple test that would only run on the full k8s case to verify that the custom template has its effect. I did verify it with a manual deploy (haven't yet with extraFiles, will do that tomorrow).

consideRatio · 2022-04-25T14:12:00Z

helm-chart/binderhub/templates/secret.yaml

+  {{- with include "binderhub.extraFiles.stringData" .Values.extraFiles }}
+  {{- . | nindent 2 }}
+  {{- end }}
+
+{{- with include "binderhub.extraFiles.data" .Values.extraFiles }}
+data:
+  {{- . | nindent 2 }}
+{{- end }}


Did you know/consider this option? The helpers seems general enough to function without being hard-copied to this helm chart, and are practically available in the helm chart already.

Suggested change

{{- with include "binderhub.extraFiles.stringData" .Values.extraFiles }}

{{- . | nindent 2 }}

{{- end }}

{{- with include "binderhub.extraFiles.data" .Values.extraFiles }}

data:

{{- . | nindent 2 }}

{{- end }}

{{- with include "jupyterhub.extraFiles.stringData" .Values.extraFiles }}

{{- . | nindent 2 }}

{{- end }}

{{- with include "jupyterhub.extraFiles.data" .Values.extraFiles }}

data:

{{- . | nindent 2 }}

{{- end }}

I will try that! I thought of it, but then wasn't sure about versions, etc. I will try it that way tomorrow

Since binderhub pins the version of JupyterHub being the dependency, this feels quite safe. Since the helpers are general enough, I'm not worried about having binderhub starting to use this - at least no more worried than needing to update the code in two places if it improves.

Can I just check, is this considered a supported use of Z2JH chart features (which therefore will be maintained in accordance with semver)?

I have two answers for that:

I would not consider any helpers in any chart public APIs, unless thoroughly documented to be otherwise

I would not consider binderhub an 'external' project, so I'm okay using these kinds of internal utilities across our internal projects, especially with the strict dependency pin we use in binderhub. If jupyterhub ever breaks this, we can vendor the previous version with a copy/paste or update.

tested and works great with the imported helpers

I appreciate the answers above about "not documented -> no commitment" and the distinction about what we recommend using to others and what we do internally.

@manics do you see an action point that would make you feel comfortable around this? Perhaps a declaration of intent for how helpers are used in z2jh? I think being explicit about such intent is fine, especially since I'm unsure about what helpers we should couple to be tracked by the changelog etc.

It's related to the idea behind dogfooding: https://en.m.wikipedia.org/wiki/Eating_your_own_dog_food

We know JupyterHub and Z2JH are used and extended by lots of users, many who we have no idea about, and BinderHub is a prime demonstration of how to build on top of Z2JH. If we're running into issues with extending Z2JH then it's probable that others will too. I think we should therefore aim to do whatever we'd tell others to do. I can't tell from this discussion what the feeling was- say it's acceptable for everything else to do this with the proviso that it's not a standard API, or try and keep it hidden from everyone?

Given the time pressure it's fine to merge this, but if the conclusion of the above discussion is that this shouldn't be copied by other applications then maybe create an issue for how to come up with a public solution?

Just chiming it that while I don't consider them public now that doesn't mean they necessarily shouldn't be - if we want to support downstream charts using extraFiles, I think the first step is adding them to the z2jh docs.

consideRatio · 2022-04-25T14:20:11Z

FWIW, I couldn't figure out how to add a simple test that would only run on the full k8s case to verify that the custom template has its effect. I did verify it with a manual deploy (haven't yet with extraFiles, will do that tomorrow).

I've also failed to make a simple test, but here is an overview of what I did in z2jh in case you wish to implement a test like that.

extraFiles chart config passed during tests
extra_files_test_command - runs inside a container to detect mounted extraFiles string literal looking for mounted test data.
pytest test running the test command above in a pod

Hm, we have a problem in CI because it's now impossible to update the helm schema and pass tests, because the same config is passed both to the current and previous version of binderhub.

I think this is inevitable to avoid issues during upgrade tests without having separate configurations. This is how I've resolved it in z2jh:

dev-config.yaml is meant for config that works in both latest stable release and current release.
dev-config-local-chart-extra-config.yaml is meant to adjust the dev-config.yaml with additions that is only valid in the latest version.

The GitHub Workflow testing the charts always run a step referencing dev-config.yaml and dev-config-local-chart-extra-config.yaml but sometimes also runs a step installing the stable release before this using only dev-config.yaml.

If the schema weren't so strict (i.e. rejecting addtionalProperties), this would be less of an issue.

Its strictness can be troublesome here, but is one of the key values of the schema I think. Rejecting unrecognized property names has helped a lot of users catch invalid configuration I believe, and also reduced the maintenance by fewer bugs being reported by invalid configuration caused by typos.

more complex, but also more general helpers are imported from the jupyterhub chart!

minrk · 2022-04-26T09:26:04Z

I went with a simple test under pytest.mark.remote. It's not 100% correct semantically (remote doesn't mean it's our helm test config), but it is correct practically (those are the tests we run against this config).

minrk · 2022-04-26T09:54:55Z

Looks like I wasn't quite right about the condition, trying a fix

opt-in test, only run when --helm is given

minrk · 2022-04-26T12:31:54Z

New test (new helm mark) now runs only when --helm is passed, verifying that extraFiles is working as intended.

binderhub/tests/conftest.py

consideRatio

LGTM besides comment with regards to removal of pytest mark.

jupyterhub/binderhub#1472 Merge pull request #1472 from minrk/helm-templates

minrk force-pushed the helm-templates branch from 561a5af to f0db8e4 Compare April 25, 2022 10:42

minrk mentioned this pull request Apr 25, 2022

Main page footer can be overridden #1471

Closed

minrk force-pushed the helm-templates branch from 7cda111 to b251afa Compare April 25, 2022 11:01

minrk force-pushed the helm-templates branch from b251afa to 0cf88e3 Compare April 25, 2022 11:14

choldgraf reviewed Apr 25, 2022

View reviewed changes

workaround CI installing same config against different schemas

d92f718

- install an extra 'old' yaml config when installing against an old binderhub version - this file should only exist during PRs that change the schema, delete immediately after merging

minrk force-pushed the helm-templates branch from 0cf88e3 to d92f718 Compare April 25, 2022 13:45

minrk force-pushed the helm-templates branch from f8d35fa to 51d204e Compare April 25, 2022 14:04

consideRatio reviewed Apr 25, 2022

View reviewed changes

import extraFiles wholesale from z2jh

fa071b4

more complex, but also more general helpers are imported from the jupyterhub chart!

minrk force-pushed the helm-templates branch from 51d204e to fa071b4 Compare April 26, 2022 09:12

minrk changed the title ~~support custom templates via helm~~ support extraFiles for binderhub pod Apr 26, 2022

minrk force-pushed the helm-templates branch from eed9f98 to 68c17f6 Compare April 26, 2022 10:12

add test for custom template

e87e25a

opt-in test, only run when --helm is given

minrk force-pushed the helm-templates branch from 68c17f6 to e87e25a Compare April 26, 2022 10:32

consideRatio reviewed Apr 26, 2022

View reviewed changes

binderhub/tests/conftest.py Show resolved Hide resolved

consideRatio reviewed Apr 26, 2022

View reviewed changes

minrk merged commit 0b49d3a into jupyterhub:master Apr 27, 2022

minrk deleted the helm-templates branch April 27, 2022 09:44

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Apr 27, 2022

[binderhub] Automatic update for commit 0.2.0-n919.he87e25a

5a4fe85

jupyterhub/binderhub#1472 Merge pull request #1472 from minrk/helm-templates

minrk mentioned this pull request Apr 27, 2022

bump binderhub d19a36a...e87e25a jupyterhub/mybinder.org-deploy#2163

Merged

manics mentioned this pull request Apr 29, 2022

Adding a donate button to the banner jupyterhub/mybinder.org-deploy#2160

Merged

consideRatio mentioned this pull request Oct 8, 2022

Should k8s-binder-k8s-hub/binderhub-chart-config-old.yaml be kept or deleted? #1520

Open

arnim mentioned this pull request Mar 9, 2023

Overview of technical work planned for the GESIS collaboration 2i2c-org/infrastructure#2120

Closed

consideRatio mentioned this pull request May 30, 2023

refactor: cleanup no longer needed old chart test config #1710

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support extraFiles for binderhub pod #1472

support extraFiles for binderhub pod #1472

minrk commented Apr 25, 2022 •

edited

Loading

minrk commented Apr 25, 2022

choldgraf left a comment

consideRatio commented Apr 25, 2022 •

edited

Loading

minrk commented Apr 25, 2022

minrk commented Apr 25, 2022

minrk commented Apr 25, 2022

consideRatio Apr 25, 2022

minrk Apr 25, 2022

consideRatio Apr 25, 2022

manics Apr 25, 2022 •

edited

Loading

minrk Apr 26, 2022

minrk Apr 26, 2022

consideRatio Apr 26, 2022

manics Apr 26, 2022

minrk Apr 27, 2022

consideRatio commented Apr 25, 2022 •

edited

Loading

minrk commented Apr 26, 2022

minrk commented Apr 26, 2022

minrk commented Apr 26, 2022

consideRatio left a comment

support extraFiles for binderhub pod #1472

support extraFiles for binderhub pod #1472

Conversation

minrk commented Apr 25, 2022 • edited Loading

minrk commented Apr 25, 2022

choldgraf left a comment

Choose a reason for hiding this comment

consideRatio commented Apr 25, 2022 • edited Loading

minrk commented Apr 25, 2022

minrk commented Apr 25, 2022

minrk commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manics Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

consideRatio commented Apr 25, 2022 • edited Loading

minrk commented Apr 26, 2022

minrk commented Apr 26, 2022

minrk commented Apr 26, 2022

consideRatio left a comment

Choose a reason for hiding this comment

minrk commented Apr 25, 2022 •

edited

Loading

consideRatio commented Apr 25, 2022 •

edited

Loading

manics Apr 25, 2022 •

edited

Loading

consideRatio commented Apr 25, 2022 •

edited

Loading