Skip to content

Conversation

manics
Copy link
Member

@manics manics commented Dec 17, 2022

This was added in #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:

{{- if or .Values.config.BinderHub.use_registry .Values.config.BinderHub.buildDockerConfig }}
kind: Secret
apiVersion: v1
metadata:
name: binder-build-docker-config
type: Opaque
data:
config.json: {{ include "buildDockerConfig" . | b64enc | quote }}
{{- end }}

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.

@manics manics added the maintenance Under the hood improvements and fixes label Dec 17, 2022
@consideRatio
Copy link
Member

consideRatio commented Dec 17, 2022

{{- /* augment our initialized docker config with buildDockerConfig */}}
{{- if .Values.config.BinderHub.buildDockerConfig }}
{{- $dockerConfig := merge $dockerConfig .Values.config.BinderHub.buildDockerConfig }}
{{- end }}

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.

if self.settings["use_registry"] or self.settings["build_docker_config"]:
push_secret = self.settings["push_secret"]
else:
push_secret = None

Something or multiple things are wrong for sure.

@consideRatio
Copy link
Member

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 consideRatio merged commit 8351995 into jupyterhub:main Dec 17, 2022
@consideRatio consideRatio added documentation and removed maintenance Under the hood improvements and fixes labels Dec 17, 2022
@consideRatio
Copy link
Member

consideRatio commented Dec 17, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants