Skip to content

Commit

Permalink
Resolve some suggestions from PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Shane Brown committed Aug 7, 2023
1 parent 59a580b commit 8e83094
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 60 deletions.
14 changes: 4 additions & 10 deletions README.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,10 @@ shows the different configuration options available:
<or>
platform: linux/arm64/v8 # an apple m1 architecture
# Multi-platform images
# Specify a list of platforms to build for, and buildrunner will build a manifest
# list image with the given platforms
# For Multi-platform images please specify a list of platforms to build for
# and buildrunner will build images with the given platforms.
# Please note that these may be built using emulated architectures which
# will be slower than native builds.
platform:
- linux/amd64
- linux/arm64/v8
Expand Down Expand Up @@ -727,13 +728,6 @@ the run step:
<or>
platform: linux/arm64/v8 # an apple m1 architecture
# Multi-platform images
# Specify a list of platforms to build for, and buildrunner will build a manifest
# list image with the given platforms
platform:
- linux/amd64
- linux/arm64/v8
# systemd does not play well with docker typically, but you can
# use this setting to tell buildrunner to set the necessary docker
# flags to get systemd to work properly:
Expand Down
77 changes: 40 additions & 37 deletions buildrunner/docker/multiplatform_image_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def __init__(self, repo: str, tags: List[str] = None):
self._repo = repo

if tags is None:
self._tags = ['latest']
self._tags = ["latest"]
else:
self._tags = tags

Expand Down Expand Up @@ -84,7 +84,7 @@ def __exit__(self, exc_type, exc_value, traceback):
for image in images:
for tag in image.tags:
LOGGER.debug(f"Removing image {image.repo}:{tag} for {name}")
docker.image.remove(f'{image.repo}:{tag}', force=True)
docker.image.remove(f"{image.repo}:{tag}", force=True)

# Removes all tagged images if keep_images is False
if self._tagged_images_names and not self._keep_images:
Expand Down Expand Up @@ -130,13 +130,13 @@ def _start_local_registry(self):
# Something changed in the registry image, so we need to update this code
assert len(ports) == 1, \
f"Expected 1 port, but got {len(ports)}"
assert isinstance(ports.get('5000/tcp')[0], dict), \
assert isinstance(ports.get("5000/tcp")[0], dict), \
f"Expected dict, but got {type(ports.get('5000/tcp')[0])}"
assert ports.get('5000/tcp')[0].get('HostIp') == '0.0.0.0', \
assert ports.get("5000/tcp")[0].get('HostIp') == "0.0.0.0", \
f"Expected HostIp to be 0.0.0.0 but got {ports.get('5000/tcp')[0].get('HostIp')}"

self._reg_ip = 'localhost'
self._reg_port = ports.get('5000/tcp')[0].get('HostPort')
self._reg_ip = "localhost"
self._reg_port = ports.get("5000/tcp")[0].get("HostPort")
self._reg_name = container.name
LOGGER.debug(f"Started local registry {self._reg_name} at {self._reg_ip}:{self._reg_port}")

Expand All @@ -145,36 +145,39 @@ def _stop_local_registry(self):
Stops and removes the local registry along with any images
"""
LOGGER.debug(f"Stopping and removing local registry {self._reg_name} at {self._reg_ip}:{self._reg_port}")
docker.stop(self._reg_name)
docker.remove(self._reg_name, volumes=True, force=True)
try:
docker.remove(self._reg_name, volumes=True, force=True)
except python_on_whales.exceptions.NoSuchContainer as err:
LOGGER.error(f"Failed to stop and remove local registry {self._reg_name}: {err}")


# pylint: disable=too-many-arguments
def build_image(self,
def build_single_image(self,
name: str,
platform: str,
push: bool = True,
path: str = '.',
file: str = 'Dockerfile',
path: str = ".",
file: str = "Dockerfile",
tags: List[str] = None,) -> None:
"""
Builds the image for the given platform
Builds a single image for the given platform
Args:
name (str): The name of the image
platform (str): The platform to build the image for (e.g. linux/amd64)
push (bool, optional): Whether to push the image to the registry. Defaults to True.
path (str, optional): The path to the Dockerfile. Defaults to '.'.
file (str, optional): The path/name of the Dockerfile (ie. <path>/Dockerfile). Defaults to 'Dockerfile'.
path (str, optional): The path to the Dockerfile. Defaults to ".".
file (str, optional): The path/name of the Dockerfile (ie. <path>/Dockerfile). Defaults to "Dockerfile".
tags (List[str], optional): The tags to apply to the image. Defaults to None.
"""
if tags is None:
tags = ['latest']
tags = ["latest"]

assert os.path.isdir(path) and os.path.exists(f'{file}'), \
assert os.path.isdir(path) and os.path.exists(f"{file}"), \
f"Either path {path}({os.path.isdir(path)}) or file " \
"'{file}'({os.path.exists(f'{file}')}) does not exist!"

tagged_names = [f'{name}:{tag}' for tag in tags]
tagged_names = [f"{name}:{tag}" for tag in tags]
LOGGER.debug(f"Building {tagged_names} for {platform}")

# Build the image with the specified tags
Expand All @@ -192,21 +195,21 @@ def build_image(self,
raise err
return image

def build(self,
def build_multiple_images(self,
platforms: List[str],
path: str = '.',
file: str = 'Dockerfile',
path: str = ".",
file: str = "Dockerfile",
name: str = None,
tags: List[str] = None,
push=True,
do_multiprocessing: bool = False) -> List[ImageInfo]:
"""
Builds the images for the given platforms
Builds multiple images for the given platforms. One image will be built for each platform.
Args:
platforms (List[str]): The platforms to build the image for (e.g. linux/amd64)
path (str, optional): The path to the Dockerfile. Defaults to '.'.
file (str, optional): The path/name of the Dockerfile (ie. <path>/Dockerfile). Defaults to 'Dockerfile'.
path (str, optional): The path to the Dockerfile. Defaults to ".".
file (str, optional): The path/name of the Dockerfile (ie. <path>/Dockerfile). Defaults to "Dockerfile".
name (str, optional): The name of the image. Defaults to None.
tags (List[str], optional): The tags to apply to the image. Defaults to None.
push (bool, optional): Whether to push the image to the registry. Defaults to True.
Expand All @@ -219,10 +222,10 @@ def build(self,
LOGGER.debug(f"Building {name}:{tags} for platforms {platforms} from {file}")

if tags is None:
tags = ['latest']
tags = ["latest"]

# Updates name to be compatible with docker
image_prefix = 'buildrunner-mp'
image_prefix = "buildrunner-mp"
santized_name = f"{image_prefix}-{name.replace('/', '-').replace(':', '-')}"
base_image_name = f"{self._reg_ip}:{self._reg_port}/{santized_name}"

Expand All @@ -234,9 +237,9 @@ def build(self,
curr_name = f"{base_image_name}-{platform.replace('/', '-')}"
LOGGER.debug(f"Building {curr_name} for {platform}")
if do_multiprocessing:
processes.append(Process(target=self.build_image, args=(curr_name, platform, push, path, file, tags)))
processes.append(Process(target=self.build_single_image, args=(curr_name, platform, push, path, file, tags)))
else:
self.build_image(curr_name, platform, push, path, file, tags)
self.build_single_image(curr_name, platform, push, path, file, tags)
self._intermediate_built_images[name].append(ImageInfo(curr_name, tags))

for proc in processes:
Expand Down Expand Up @@ -275,13 +278,13 @@ def push(self, name: str, dest_names: List[str] = None) -> None:
if dest_names is None:
dest_names = name
for tag in src_images[0].tags:
tagged_names.append(f'{dest_names}:{tag}')
tagged_names.append(f"{dest_names}:{tag}")
else:
tagged_names = dest_names

for image in src_images:
for tag in image.tags:
src_names.append(f'{image.repo}:{tag}')
src_names.append(f"{image.repo}:{tag}")

timeout_seconds = initial_timeout_seconds
while retries > 0:
Expand Down Expand Up @@ -319,7 +322,7 @@ def _find_native_platform_images(self, name: str) -> str:
host_os = system()
host_arch = machine()
LOGGER.debug(f"Finding native platform for {name} for {host_os}/{host_arch}")
pattern = f'{host_os}-{host_arch}'
pattern = f"{host_os}-{host_arch}"

# No images built for this name
if name not in self._intermediate_built_images.keys() \
Expand All @@ -330,8 +333,8 @@ def _find_native_platform_images(self, name: str) -> str:

# No matches found, change os
if match_platform == []:
if host_os == 'Darwin':
pattern = f'linux-{host_arch}'
if host_os == "Darwin":
pattern = f"linux-{host_arch}"
match_platform = [image for image in self._intermediate_built_images[name] if image.repo.endswith(pattern)]

assert len(match_platform) <= 1, f"Found more than one match for {name} and {pattern}: {match_platform}"
Expand All @@ -349,13 +352,13 @@ def tag_single_platform(self, name: str, tags: List[str] = None, dest_name: str
Args:
name (str): The name of the image to load
tags (List[str], optional): The tags to load. Defaults to 'latest'.
dest_name (str, optional): The name to load the image as. Defaults to the 'name' arg.
tags (List[str], optional): The tags to load. Defaults to "latest".
dest_name (str, optional): The name to load the image as. Defaults to the "name" arg.
"""
# This is to handle pylint's "dangerous-default-value" error
if tags is None:
tags = ['latest']
LOGGER.debug(f'Tagging {name} with tags {tags} - Dest name: {dest_name}')
tags = ["latest"]
LOGGER.debug(f"Tagging {name} with tags {tags} - Dest name: {dest_name}")
source_image = self._find_native_platform_images(name)
if dest_name is None:
dest_name = name
Expand All @@ -369,7 +372,7 @@ def tag_single_platform(self, name: str, tags: List[str] = None, dest_name: str
try:
docker.pull(image)
for tag in tags:
dest_tag = f'{dest_name}:{tag}'
dest_tag = f"{dest_name}:{tag}"
docker.tag(image, dest_tag)
LOGGER.debug(f"Tagged {image} as {dest_tag}")
self._tagged_images_names[name].append(dest_tag)
Expand Down
4 changes: 2 additions & 2 deletions buildrunner/steprunner/tasks/build.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ def run(self, context):
name=self.get_unique_build_name(),)

assert len(built_images) == len(self.platform), \
f"Number of built images ({len(built_images)}) does not match " \
"the number of platforms ({len(self.platform)})"
f'Number of built images ({len(built_images)}) does not match ' \
'the number of platforms ({len(self.platform)})'

else:
exit_code = builder.build(
Expand Down
24 changes: 13 additions & 11 deletions tests/test_multiplatform.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

TEST_DIR = os.path.basename(os.path.dirname(__file__))

# FIXME: These tests can be broken if a custom buildx builder is set as default # pylint: disable=fixme

def actual_images_match_expected(actual_images, expected_images) -> List[str]:
missing_images = []
found = False
Expand Down Expand Up @@ -133,7 +135,7 @@ def test_tag_single_platform(name, platforms, expected_image_names):
tag='latest'
test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=name,
built_images = mp.build_multiple_images(name=name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -166,7 +168,7 @@ def test_tag_single_platform_multiple_tags(name, platforms, expected_image_names
tags=['latest', '0.1.0']
test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=name,
built_images = mp.build_multiple_images(name=name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -201,7 +203,7 @@ def test_tag_single_platform_keep_images(name, platforms, expected_image_names):
test_path = f'{TEST_DIR}/test-files/multiplatform'
try:
with MultiplatformImageBuilder(keep_images=True) as mp:
built_images = mp.build(name=name,
built_images = mp.build_multiple_images(name=name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -241,7 +243,7 @@ def test_push():

test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=build_name,
built_images = mp.build_multiple_images(name=build_name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -281,7 +283,7 @@ def test_push_with_dest_names():

test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=build_name,
built_images = mp.build_multiple_images(name=build_name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -319,10 +321,10 @@ def test_push_with_dest_names():
)
])
def test_build(name, platforms, expected_image_names):
with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_image'):
with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_single_image'):
test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=name,
built_images = mp.build_multiple_images(name=name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand All @@ -344,17 +346,17 @@ def test_build_multiple_builds():
expected_image_names2 = ['test-build-multi-image-2002-linux-amd64', 'test-build-multi-image-2002-linux-arm64']

test_path = f'{TEST_DIR}/test-files/multiplatform'
with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_image'):
with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_single_image'):
with MultiplatformImageBuilder() as mp:
# Build set 1
built_images1 = mp.build(name=name1,
built_images1 = mp.build_multiple_images(name=name1,
platforms=platforms1,
path=test_path,
file=f'{test_path}/Dockerfile',
do_multiprocessing=False)

# Build set 2
built_images2 = mp.build(name=name2,
built_images2 = mp.build_multiple_images(name=name2,
platforms=platforms2,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down Expand Up @@ -387,7 +389,7 @@ def test_build_multiple_builds():
def test_build_with_tags(name, tags, platforms, expected_image_names):
test_path = f'{TEST_DIR}/test-files/multiplatform'
with MultiplatformImageBuilder() as mp:
built_images = mp.build(name=name,
built_images = mp.build_multiple_images(name=name,
platforms=platforms,
path=test_path,
file=f'{test_path}/Dockerfile',
Expand Down

0 comments on commit 8e83094

Please sign in to comment.