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

hub.extraConfig as a string is not supported - update docs and remove potential related logic #2063

Closed
consideRatio opened this issue Feb 24, 2021 · 1 comment · Fixed by #2307

Comments

@consideRatio
Copy link
Member

As reported by @danielballan in #2062 (comment) (point 1), it may not be supported for us to set hub.extraConfig to a string.

hub:
  extraConfig: |
    import time
    c.Spawner.environment += {
       "CURRENT_TIME": str(time.time())
    }

The documentation claims that extraConfig accepts either a dict of strings or a single string (of Python code), but it actually only accepts the first one. If you give it a single string, following the documented example, it warns
coalesce.go:196: warning: cannot overwrite table with non table for extraConfig (map[])

and it does not take effect. See #653 which seems to observe the same issue.

Action point

  1. Reproduce and conclude that helm 3 and 0.11.1 that this doesn't work.

    hub:
      extraConfig: |
        print("THIS SHOULD BE SEEN IN LOGS ON STARTUP, IS IT?")
@consideRatio
Copy link
Member Author

consideRatio commented Jul 6, 2021

We have apparently made it stop working entirely no matter what in 1.0.0 where we introduced schema validation.

helm template --repo https://jupyterhub.github.io/helm-chart/ jupyterhub --set hub.extraConfig="print('test')"

coalesce.go:200: warning: cannot overwrite table with non table for extraConfig (map[])
Error: values don't meet the specifications of the schema(s) in the following chart(s):
jupyterhub:
- hub.extraConfig: Invalid type. Expected: object, given: string

I suggest we drop this as: a) it is already dropped unintentionally, b) it is a bad practice as one can only have one extraConfig entry, and c) it increase the complexity of the helm chart, d) it was deprecated as of z2jh 0.8.0

As an action point, we should make sure we drop the complexity related to allowing this config to be a string.

  • Remove documentation about hub.extraConfig as a string (advanced.md includes this!)
  • Remove logic if there is such to support hub.extraConfig being a string rather than a dict.

@consideRatio consideRatio changed the title Is passing a string still really supported for hub.extraConfig? hub.extraConfig as a string is not supported - update docs and remove potential related logic Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant