jupyterhub / binderhub

Run your code in the cloud, with technology so advanced, it feels like magic!
https://binderhub.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.54k stars 388 forks source link

Warn about build_docker_config being a Helm Chart option not a BinderHub option #1592

Closed manics closed 1 year ago

manics commented 1 year ago

This was added in https://github.com/jupyterhub/binderhub/pull/1255

As far as I can see the value of c.BinderHub.build_docker_config isn't used by the BinderHub Python app, only it's presence is checked:

$ git grep build_docker_config origin/main

origin/main:binderhub/app.py:    build_docker_config = Dict(
origin/main:binderhub/app.py:                "build_docker_config": self.build_docker_config,
origin/main:binderhub/builder.py:        if self.settings["use_registry"] or self.settings["build_docker_config"]:

Instead it's used in the Helm Chart secret: https://github.com/jupyterhub/binderhub/blob/2ba938bf20201ff864b3a6c473f66f30fbef92bc/helm-chart/binderhub/templates/secret.yaml#L34-L42

This adds a warning message to help avoid confusion.

Note I don't understand the Helm chart code.... the secret refers to config.BinderHub.buildDockerConfig instead of config.BinderHub.build_docker_config.

consideRatio commented 1 year ago

https://github.com/jupyterhub/binderhub/blob/2ba938bf20201ff864b3a6c473f66f30fbef92bc/helm-chart/binderhub/templates/_helpers.tpl#L42-L45

This segment seems like a bug, or it is at least a notable issue because its simply separate from providing config.BinderHub.build_docker_config in values. Whats under .config should be pure passthrough - so buildDockerConfig won't map to build_docker_config.

So currently, either:

  1. The Helm chart's logic is respected with config.BinderHub.buildDockerConfig, if that specific value is provided.
  2. The python logic is respected with config.BinderHub.build_docker_config set, but the impact of doing that is trivial - it will just make an explicitly set push_secret be consumed if also use_registry is False.

https://github.com/jupyterhub/binderhub/blob/2ba938bf20201ff864b3a6c473f66f30fbef92bc/binderhub/builder.py#L440-L443

Something or multiple things are wrong for sure.

consideRatio commented 1 year ago

If we can reduce the complexity of this, that would be great - to me this seems bugged overall, so a breaking change would be acceptable in my mind as a bugfix.

consideRatio commented 1 year ago

@manics I changed the maintenance label to a documentation label as this PR was pure documentation/inline comments - even though its documentation related to maintenance efforts. Feel free to switch back, i see upsides with either approach!

I have leaned towards labelling PRs as documentation if no code change, guided by the idea that it makes it easier to dismiss the PR as something that couldn't possibly have caused a regression or a change in behavior etc.