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 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
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.

Is it worth pushing this up a level to ContainerEngine under a more generic name? It shouldn't be a breaking change since it'll be ignored by default.

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 if we change the container engine, the parameters passed to this necessarily need to change, as these aren't portable across engines. so you may want to pass different params to podman vs here. so I think it's useful to just leave them over here

[],
yuvipanda marked this conversation as resolved.
Show resolved Hide resolved
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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we'll have to see how it goes. But buildkit does give us a lot more knobs here - https://docs.docker.com/build/cache/backends/.

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):
)


yuvipanda marked this conversation as resolved.
Show resolved Hide resolved
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