-
Notifications
You must be signed in to change notification settings - Fork 370
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
d0d87c8
1ca89b8
258daa7
93cb0d7
77ed499
743c9f9
b1cedc4
8e20b60
a03bf74
d940706
5b96156
d7968bb
6df3ec6
087c332
07ef92e
bd61590
34399f8
bd39320
b243cb6
6484673
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
|
@@ -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( | ||
{}, | ||
|
@@ -69,6 +74,14 @@ class DockerEngine(ContainerEngine): | |
config=True, | ||
) | ||
|
||
extra_buildx_build_args = List( | ||
[], | ||
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: | ||
|
@@ -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"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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