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

Switch to using CLI for everything except running the container #1421

Merged
merged 35 commits into from
Mar 6, 2025
Merged
Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
ac0def4
Use CLI to push docker image
yuvipanda Feb 26, 2025
268e187
Properly set DOCKER_CONFIG to be a dir, not a file
yuvipanda Feb 26, 2025
3bdc004
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 26, 2025
364a189
Add a local registry in github actions for us to test with
yuvipanda Feb 28, 2025
82e049f
Add a docker registry integration test with real registry
yuvipanda Feb 28, 2025
8444a28
Explicitly pull registry image before trying to run
yuvipanda Feb 28, 2025
69e67a2
Don't need -it for docker run here
yuvipanda Feb 28, 2025
a8369be
Use custom dind for talking to insecure registry
yuvipanda Feb 28, 2025
9f6fbf7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
576b896
Stop using docker-py for anything
yuvipanda Feb 28, 2025
48da2c1
Re-add dockerpy for using exclude patterns
yuvipanda Feb 28, 2025
a0f02ce
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
2d9a350
Inspect returns a list of dicts
yuvipanda Feb 28, 2025
af1050f
Re-add docker import for container running
yuvipanda Feb 28, 2025
5aaf26a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
9753570
Pass cert_dir removal some of the time
yuvipanda Feb 28, 2025
c04652e
Replace mocked find_image tests with real ones
yuvipanda Feb 28, 2025
c9b78be
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
0bc550a
Add a .gitignore for any generated tmp certs
yuvipanda Feb 28, 2025
b854c6c
Document why we put certs in current dir
yuvipanda Feb 28, 2025
3e458e3
Remove a couple more tests
yuvipanda Feb 28, 2025
1bd3d27
Restore env vars after test
yuvipanda Feb 28, 2025
c294236
Remove some more tests
yuvipanda Feb 28, 2025
aa948e0
Test login to registry as well
yuvipanda Feb 28, 2025
ad29ffd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
2fece29
Add validation for registry_credentials
yuvipanda Feb 28, 2025
0f68056
Intentionally set auth when checking if image exists in registry
yuvipanda Feb 28, 2025
e066ca0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
07e97ee
Turn push and load into params for the build arg
yuvipanda Feb 28, 2025
2bd8478
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 28, 2025
081c3c4
Make build and push into explicit only args
yuvipanda Feb 28, 2025
56cf063
Don't pass --no-run to find tests
yuvipanda Feb 28, 2025
ce35f47
Actually remove push method
yuvipanda Feb 28, 2025
d3facba
Fix documentation for `--push`
yuvipanda Feb 28, 2025
342eea9
Move login command inside try so we clear env correctly
yuvipanda Feb 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 41 additions & 11 deletions repo2docker/docker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@
Docker container engine for repo2docker
"""

import os
import shutil
import subprocess
import tarfile
import tempfile
from contextlib import contextmanager
from pathlib import Path

from iso8601 import parse_date
from traitlets import Dict, List, Unicode
Expand Down Expand Up @@ -58,7 +62,7 @@ class DockerEngine(ContainerEngine):
https://docker-py.readthedocs.io/en/4.2.0/api.html#module-docker.api.build
"""

string_output = False
string_output = True

extra_init_args = Dict(
{},
Expand Down Expand Up @@ -141,18 +145,12 @@ def build(

args += [d]

for line in execute_cmd(args, True):
# Simulate structured JSON output from buildx build, since we
# do get structured json output from pushing and running
yield {"stream": line}
yield from execute_cmd(args, True)
else:
# Assume 'path' is passed in
args += [path]

for line in execute_cmd(args, True):
# Simulate structured JSON output from buildx build, since we
# do get structured json output from pushing and running
yield {"stream": line}
yield from execute_cmd(args, True)

def images(self):
images = self._apiclient.images()
Expand All @@ -162,10 +160,42 @@ def inspect_image(self, image):
image = self._apiclient.inspect_image(image)
return Image(tags=image["RepoTags"], config=image["Config"])

@contextmanager
def docker_login(self, username, password, registry):
# Determine existing DOCKER_CONFIG
dc_path = Path(
os.environ.get("DOCKER_CONFIG", os.path.expanduser("~/.docker/config.json"))
)

with tempfile.TemporaryDirectory() as d:
new_dc_path = Path(d) / "config.json"
if dc_path.exists():
# If there is an existing DOCKER_CONFIG, copy it to new location so we inherit
# whatever configuration the user has already set
shutil.copy2(dc_path, new_dc_path)

env = os.environ.copy()
subprocess.check_call(
# FIXME: This should be using --password-stdin instead
Copy link
Member

@manics manics Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to manipulate the config.json file directly:

json.dumps({"auths":{"<might need a scheme here://>registry.host":{"auth":b64encode(f"{user}:{token}".encode()).decode()}}})

It's also what we do to create the BinderHub secret:
https://github.com/jupyterhub/binderhub/blob/579dd78430422bae896c90d69ffad637d9fcfc26/helm-chart/binderhub/templates/_helpers.tpl#L40

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manics I did consider that, but then we'll have to merge the existing ~/.docker/config file with the new setup carefully. I would rather just outsource it to docker login like this instead.

[
"docker",
"login",
"--username",
username,
"--password",
password,
registry,
],
env=env,
)
yield

def push(self, image_spec):
if self.registry_credentials:
self._apiclient.login(**self.registry_credentials)
return self._apiclient.push(image_spec, stream=True)
with self.docker_login(**self.registry_credentials):
yield from execute_cmd(["docker", "push", image_spec], capture=True)
else:
yield from execute_cmd(["docker", "push", image_spec], capture=True)

def run(
self,
Expand Down
Loading