Skip to content
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.

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

fix default value of hub_url in config #1139

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bitnik
Copy link
Collaborator

@bitnik bitnik commented Aug 10, 2020

In gesiscss/persistent_binderhub#5 we found out that if someone installs BinderHub with auth enabled,
deployment fails with
Error: render error in "persistent_binderhub/charts/binderhub/templates/deployment.yaml": template: persistent_binderhub/charts/binderhub/templates/deployment.yaml:98:74: executing "persistent_binderhub/charts/binderhub/templates/deployment.yaml" at <"/">: invalid value; expected string

The error indicates

value: {{ (print (.Values.config.BinderHub.hub_url | trimSuffix "/") "/hub/api/") }}
and it happens only if the deployment is not an upgrade from a standard BinderHub deployment (auth disabled), but it is an installation of BinderHub directly with authentication included. The reason is that on cloud-based environments BinderHub.hub_url is missing during the first round of the installation (https://binderhub.readthedocs.io/en/latest/zero-to-binderhub/setup-binderhub.html#install-binderhub), because it is provided after the first installation when JupyerterHub is ready (https://binderhub.readthedocs.io/en/latest/zero-to-binderhub/setup-binderhub.html#connect-binderhub-and-jupyterhub).

This PR fixes the default value of BinderHub.hub_url in helm config, so helm won't complain about it during the first installation and then user can set BinderHub.hub_url when it is available. It also fixes it binderhub_config.py, so it uses default value of hub_url "" instead of LazyConfigValue.

@bitnik
Copy link
Collaborator Author

bitnik commented Aug 10, 2020

Actually now I am thinking that instead of my first changes, it would be better to set the default value in values.yaml as

config:
  BinderHub:
    hub_url: ''

what do you think?

@bitnik
Copy link
Collaborator Author

bitnik commented Sep 8, 2020

@minrk I am not sure how to continue here. Could you do another review?

@consideRatio
Copy link
Member

consideRatio commented Oct 10, 2021

Actually now I am thinking that instead of my first changes, it would be better to set the default value in values.yaml as

config:
  BinderHub:
    hub_url: ''

what do you think?

I agree! That is what we do in z2jh as well I think, and it makes it clearer to the reader of the config than if the default value is different from the configured empty string etc.

@bitnik this LGTM, sorry you have had to experience such a delay on getting review help. I'm 100% committed to helping this get merged and could even take over if you want. Thanks for your work in BinderHub!!!!!! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code:helm-chart Helm template changes. code:python Python changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants