Move custom BinderSpawner config to traitlets

[yuvipanda]

Currently, we have two config variables that are specified in jupyterhub.custom
so they can be read by code in the JupyterHub custom config. However, we can
do this in a much more structured way by using actual traitlets. Better validation,
documentation and 'cleaner'.

This also helps move config away from z2jh specific things (z2jh.get_config)
towards the more generic traitlets.

[yuvipanda]

@consideRatio you might like this, given it removes entries from custom config and moves them towards something with a better defined schema :)

[consideRatio]

I think this is an absolutely great refactoring, it embeds logic in the Helm chart to the Python code in BinderHub application running in the binder pod and the BinderSpawner running in the hub pod. Nice!

There is a question about how to handle this as a breaking change though. I wouldn't hesitate merging if you went for a NOTES.txt rendered message if someone was passing anything to the deprecated Helm chart config values still. To do that, first make sure the schema tolerates that. In practice, I figure:

1. Let values.yaml define a blank entry still.
2. Let schema still list it under properties so additionalProperties: false can be used, but remove it from being an required entry.
3. Let NOTES.txt check if the dictionary is truthy, then print a message and pipe it to fail.
   I'd like to see the pattern used in Z2JH's NOTES.txt be reused for this as it makes it very clear.

[yuvipanda]

@consideRatio I think I've done the changes you have asked for! Very nice way to make breaking changes, thanks for building out the system and pointing me to it!

[consideRatio]

Minor fix, otherwise LGTM.

[consideRatio]

Suggested change

	additionalProperties: false
	additionalProperties: true

Without this being true, a user won't be able to get informed via our NOTES.txt, because helm will fail even before that if someone passed something nested under this.

[consideRatio]

@yuvipanda i dont think you tested the deprecation logic, this must allow unlisted properties as well i think!

Can you also set the additionalProperties to true explicitly on the other deprecated entry for consistency of having an explicit choice for all our objects?

[consideRatio]

Doing this, and you can test the deprecation logic

tools/generate-schema...
helm template bh helm-chart/binderhub --set cors.stuff=...

[consideRatio]

Resolved by my small schema fixes commit.

[consideRatio]

@yuvipanda i reviewed and then i saw a force push making my review potentially irrelevant, not sure what changed - I'll assume I won't need to re-view it and there was just some small detail.

[MridulS]

A quick clarification regarding this: Will this break downstream packages which define custom Spawners and not use BinderSpawner?

[consideRatio]

Depends, what do they inherit from? If they inherit from BinderSpawner, everything should be fine I think.

[consideRatio]

This config is like writing c.BinderSpawner.auth_enabled = False and will influence a MyCustomBinderSpawner class as well as long as it inherits from BinderSpawner

[consideRatio]

Note there is a helpful message during this breaking upgrade.

During an upgrade, you will be prompted that Helm chart config setting if auth is enabled or not should be done with new properties - namely this.

[MridulS]

Thanks! We inherit from KubeSpawner so need to fix that up for Persistent BinderHub :)

[consideRatio]

Test failure is relevant!

[yuvipanda]

@consideRatio test failures fixed!

[consideRatio]

I went ahead and tested the schema after making some tweaks, and pushed a commit. This now LGTM.

$ helm template bh helm-chart/binderhub/ --set jupyterhub.custom.cors.allowedOrigin=asdf
Error: template: binderhub/templates/NOTES.txt:186:4: executing "binderhub/templates/NOTES.txt" at <fail (print $breaking_title $breaking)>: error calling fail: 

#################################################################################
######   BREAKING: The config values passed contained no longer accepted    #####
######             options. See the messages below for more details.        #####
######                                                                      #####
######             To verify your updated config is accepted, you can use   #####
######             the `helm template` command.                             #####
#################################################################################

RENAMED: jupyterhub.custom.cors.allowedOrigin has been renamed to jupyterhub.hub.config.BinderSpawner.cors_allow_origin

Use --debug flag to render out invalid YAML



$ helm template bh helm-chart/binderhub/ --set cors.allowedOrigin=asd
Error: template: binderhub/templates/NOTES.txt:186:4: executing "binderhub/templates/NOTES.txt" at <fail (print $breaking_title $breaking)>: error calling fail: 

#################################################################################
######   BREAKING: The config values passed contained no longer accepted    #####
######             options. See the messages below for more details.        #####
######                                                                      #####
######             To verify your updated config is accepted, you can use   #####
######             the `helm template` command.                             #####
#################################################################################

RENAMED: cors.allowedOrigin has been renamed to config.BinderHub.cors_allow_origin

Use --debug flag to render out invalid YAML

[yuvipanda]

Thanks a lot, @consideRatio! Feels good to be writing code for binderhub again :)