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

Shell out to docker buildx build to build images #1402

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
73 changes: 55 additions & 18 deletions repo2docker/docker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@
Docker container engine for repo2docker
"""

import shutil
import tarfile
import tempfile

from iso8601 import parse_date
from traitlets import Dict
from traitlets import Dict, List, Unicode

import docker

from .engine import Container, ContainerEngine, ContainerEngineException, Image
from .utils import execute_cmd


class DockerContainer(Container):
Expand Down Expand Up @@ -53,7 +58,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 All @@ -69,6 +74,14 @@ class DockerEngine(ContainerEngine):
config=True,
)

extra_buildx_build_args = List(
[],
Copy link
Member

Choose a reason for hiding this comment

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

If you want to specify the type of items in the list, give a single TraitType instance here (not list):

Suggested change
[],
Unicode(),

help="""
Extra commandline arguments to pass to `docker buildx build` when building the image.
""",
config=True,
)

def __init__(self, *, parent):
super().__init__(parent=parent)
try:
Expand All @@ -94,22 +107,46 @@ def build(
platform=None,
**kwargs,
):
return self._apiclient.build(
buildargs=buildargs,
cache_from=cache_from,
container_limits=container_limits,
forcerm=True,
rm=True,
tag=tag,
custom_context=custom_context,
decode=True,
dockerfile=dockerfile,
fileobj=fileobj,
path=path,
labels=labels,
platform=platform,
**kwargs,
)
if not shutil.which("docker"):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like rm and forcerm don't have equivalents. Just noting that this is intentional. Are there likely to be significant changes in the cache behavior with buildkit we need to pay attention to?

raise RuntimeError("The docker commandline client must be installed")
args = ["docker", "buildx", "build", "--progress", "plain", "--load"]
if buildargs:
for k, v in buildargs.items():
args += ["--build-arg", f"{k}={v}"]
minrk marked this conversation as resolved.
Show resolved Hide resolved

if cache_from:
for cf in cache_from:
args += ["--cache-from", cf]

if dockerfile:
args += ["--file", dockerfile]

if tag:
args += ["--tag", tag]

if labels:
for k, v in labels.items():
args += ["--label", f"{k}={v}"]

if platform:
args += ["--platform", platform]

# place extra args right *before* the path
args += self.extra_buildx_build_args

if fileobj:
with tempfile.TemporaryDirectory() as d:
tarf = tarfile.open(fileobj=fileobj)
tarf.extractall(d)

args += [d]

yield from execute_cmd(args, True)
else:
# Assume 'path' is passed in
args += [path]

yield from execute_cmd(args, True)

def images(self):
images = self._apiclient.images()
Expand Down
35 changes: 0 additions & 35 deletions tests/unit/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,6 @@ def test_local_dir_image_name(repo_with_content):
)


def test_build_kwargs(repo_with_content):
upstream, sha1 = repo_with_content
argv = [upstream]
app = make_r2d(argv)
app.extra_build_kwargs = {"somekey": "somevalue"}

with patch.object(docker.APIClient, "build") as builds:
builds.return_value = []
app.build()
builds.assert_called_once()
args, kwargs = builds.call_args
assert "somekey" in kwargs
assert kwargs["somekey"] == "somevalue"


def test_run_kwargs(repo_with_content):
upstream, sha1 = repo_with_content
argv = [upstream]
Expand All @@ -107,26 +92,6 @@ def test_run_kwargs(repo_with_content):
assert kwargs["somekey"] == "somevalue"


def test_root_not_allowed():
with TemporaryDirectory() as src, patch("os.geteuid") as geteuid:
geteuid.return_value = 0
argv = [src]
with pytest.raises(SystemExit) as exc:
app = make_r2d(argv)
assert exc.code == 1

with pytest.raises(ValueError):
app = Repo2Docker(repo=src, run=False)
app.build()

app = Repo2Docker(repo=src, user_id=1000, user_name="jovyan", run=False)
app.initialize()
with patch.object(docker.APIClient, "build") as builds:
builds.return_value = []
app.build()
builds.assert_called_once()


def test_dryrun_works_without_docker(tmpdir, capsys):
with chdir(tmpdir):
with patch.object(docker, "APIClient") as client:
Expand Down
Loading