Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions .github/workflows/bakery-build-native.yml
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ jobs:
- name: Build
env:
GIT_SHA: ${{ github.sha }}
# FIXME: Currently pushes to ghcr.io for caching. Needs to be conditional
run: |
PLATFORM=${BUILD_PLATFORM#linux/} \
bakery build \
Expand All @@ -212,8 +211,9 @@ jobs:
--cache-registry "ghcr.io/${{ github.repository_owner }}" \
--temp-registry "ghcr.io/${{ github.repository_owner }}" \
--metadata-file "./${{ matrix.img.image }}-${{ matrix.img.version }}-${{ steps.normalize-platform.outputs.platform }}-metadata.json" \
--context ${{ inputs.context }} \
--push
${{ inputs.push && '--push-cache' || '' }} \
--push \
--context ${{ inputs.context }}
- name: Test
run: |
PLATFORM=${BUILD_PLATFORM#linux/} \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure what this variable is used for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it was something I tried before steps.normalize-platform.outputs.platform and it wasn't removed.

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/bakery-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,13 @@ jobs:
- name: Build
env:
GIT_SHA: ${{ github.sha }}
# FIXME: Currently pushes to ghcr.io for caching. Needs to be conditional
run: |
bakery build --load \
--image-name '^${{ matrix.img.image }}$' \
--image-version ${{ matrix.img.version }} \
--dev-versions ${{ inputs.dev-versions }} \
--cache-registry "ghcr.io/${{ github.repository_owner }}" \
${{ inputs.push && '--push-cache' || '' }} \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this as part of the initial build step.

We could easily move the --push-cache flag to after the test step or even during the push step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine where it is. Test failures are something I would consider being independent of the validity of the build cache. The cache, at least as I understand, should not push on a failed build anyways.

--context ${{ inputs.context }}

- name: Test
Expand Down
8 changes: 8 additions & 0 deletions posit-bakery/posit_bakery/cli/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ def build(
rich_help_panel=RichHelpPanelEnum.BUILD_CONFIGURATION_AND_OUTPUTS,
),
] = False,
push_cache: Annotated[
Optional[bool],
typer.Option(
help="Push the build cache to the cache registry without pushing the image.",
rich_help_panel=RichHelpPanelEnum.BUILD_CONFIGURATION_AND_OUTPUTS,
),
] = False,
clean: Annotated[
Optional[bool],
typer.Option(
Expand Down Expand Up @@ -205,6 +212,7 @@ def build(
config.build_targets(
load=load,
push=push,
push_cache=push_cache,
cache=cache,
platforms=image_platform,
strategy=strategy,
Expand Down
6 changes: 5 additions & 1 deletion posit-bakery/posit_bakery/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@ def build_targets(
self,
load: bool = True,
push: bool = False,
push_cache: bool = False,
cache: bool = True,
platforms: list[str] | None = None,
strategy: ImageBuildStrategy = ImageBuildStrategy.BAKE,
Expand All @@ -744,6 +745,7 @@ def build_targets(

:param load: If True, load the built images into the local Docker daemon.
:param push: If True, push the built images to the configured registries.
:param push_cache: If True, push the build cache to the cache registry without pushing images.
:param cache: If True, use the build cache when building images.
:param platforms: Optional list of platforms to build for. If None, builds for the configuration specified
platform.
Expand All @@ -752,7 +754,9 @@ def build_targets(
:param fail_fast: If True, stop building targets on the first failure.
"""
if strategy == ImageBuildStrategy.BAKE:
bake_plan = BakePlan.from_image_targets(context=self.base_path, image_targets=self.targets)
bake_plan = BakePlan.from_image_targets(
context=self.base_path, image_targets=self.targets, push_cache=push_cache
)
set_opts = None
if self.settings.temp_registry is not None and push:
set_opts = {
Expand Down
22 changes: 16 additions & 6 deletions posit-bakery/posit_bakery/image/bake/bake.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,19 @@ def serialize_path(value: Path | str) -> str:
return str(value)

@classmethod
def from_image_target(cls, image_target: ImageTarget) -> "BakeTarget":
def from_image_target(cls, image_target: ImageTarget, push_cache: bool = False) -> "BakeTarget":
"""Create a BakeTarget from an ImageTarget."""
kwargs = {"tags": image_target.tags}
platforms = image_target.image_os.platforms if image_target.image_os is not None else DEFAULT_PLATFORMS

if image_target.cache_name is not None:
kwargs["cache_from"] = [{"type": "registry", "ref": image_target.cache_name}]
kwargs["cache_to"] = [{"type": "registry", "ref": image_target.cache_name, "mode": "max"}]
cache_name = image_target.cache_name
# Append platform suffix to cache name
platform_suffix = "-".join(p.removeprefix("linux/").replace("/", "-") for p in platforms)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can generate a suffix like -amd64-arm64 for a multi-platform image build for bake.
I don't know how much sense this makes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I don't think this will cover us properly. image_target.image_os.platforms will get all supported platforms for the OS, not necessarily the same ones we're targeting with a build. Currently, the targeted platforms are only passed in BakePlan.build. This will require some finagling.

One possible method I can think of is to move setting the cache_to/cache_from from the BakePlan.build method. In BakeTarget, we could move cache configuration to a method called configure_cache. It could take platforms as an argument along with other stuff like push and perform this same construction and concatenation of the platform_suffix to cache_name with the appropriate platforms given to BakePlan.build. From the method, we could set the cache_to and cache_from to ensure they are properly constructed. Does that make sense as a flow? The logic seems otherwise correct, this just helps us ensure that the native builder caches remain separated while also allowing for emulated builders to utilize the cache still.

cache_name = f"{cache_name}-{platform_suffix}"
kwargs["cache_from"] = [{"type": "registry", "ref": cache_name}]
if push_cache:
kwargs["cache_to"] = [{"type": "registry", "ref": cache_name, "mode": "max"}]

if image_target.temp_name is not None:
kwargs["tags"] = [image_target.temp_name.rsplit(":", 1)[0]]
Expand All @@ -107,7 +114,7 @@ def from_image_target(cls, image_target: ImageTarget) -> "BakeTarget":
image_os=image_target.image_os.name if image_target.image_os else None,
dockerfile=image_target.containerfile,
labels=image_target.labels,
platforms=image_target.image_os.platforms if image_target.image_os is not None else DEFAULT_PLATFORMS,
platforms=platforms,
**kwargs,
)

Expand Down Expand Up @@ -151,11 +158,14 @@ def update_groups(
return groups

@classmethod
def from_image_targets(cls, context: Path, image_targets: list[ImageTarget]) -> "BakePlan":
def from_image_targets(
cls, context: Path, image_targets: list[ImageTarget], push_cache: bool = False
) -> "BakePlan":
"""Create a BakePlan from a list of ImageTarget objects.

:param context: The absolute path to the build context directory.
:param image_targets: A list of ImageTarget objects to include in the bake plan.
:param push_cache: Whether to push build cache to the cache registry.

:return: A BakePlan object containing the context, groups, and targets.
"""
Expand All @@ -165,7 +175,7 @@ def from_image_targets(cls, context: Path, image_targets: list[ImageTarget]) ->
targets: dict[str, BakeTarget] = {}

for image_target in image_targets:
bake_target = BakeTarget.from_image_target(image_target=image_target)
bake_target = BakeTarget.from_image_target(image_target=image_target, push_cache=push_cache)
groups = cls.update_groups(
groups=groups,
uid=image_target.uid,
Expand Down
7 changes: 6 additions & 1 deletion posit-bakery/posit_bakery/image/image_target.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,12 @@ def build(
cache_from = None
cache_to = None
if self.cache_name is not None:
cache_from = f"type=registry,ref={self.cache_name}"
cache_name = self.cache_name
# Append platform suffix to cache name
build_platforms = platforms or self.image_os.platforms
platform_suffix = "-".join(p.removeprefix("linux/").replace("/", "-") for p in build_platforms)
cache_name = f"{cache_name}-{platform_suffix}"
cache_from = f"type=registry,ref={cache_name}"
cache_to = f"{cache_from},mode=max"

if isinstance(metadata_file, bool) and metadata_file:
Expand Down
9 changes: 9 additions & 0 deletions posit-bakery/test/features/cli/build.feature
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ Feature: build
Then The command succeeds
* the bake plan is valid

Scenario: Generating a buildkit bake plan with push-cache flag
Given I call bakery build
* in a temp basic context
* with the arguments:
| --plan | --push-cache |
When I execute the command
Then The command succeeds
* the bake plan is valid

Scenario: Generating a buildkit bake plan with git commit
Given I call bakery build
* in the basic context
Expand Down
17 changes: 17 additions & 0 deletions posit-bakery/test/image/bake/test_bake.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,23 @@ def test_from_image_targets_with_cache_registry(self, get_expected_plan, get_con
assert plan.bake_file == resource_path / suite / ".bakery-bake.json"
assert expected_plan.read_text().strip() == output

@pytest.mark.parametrize("suite", SUCCESS_SUITES)
def test_from_image_targets_with_cache_registry_push_cache(self, get_config_obj, suite):
"""Test that bake plans include cache_to when push_cache=True."""
config_obj = get_config_obj(suite)

settings = ImageTargetSettings(cache_registry="ghcr.io/posit-dev")
for target in config_obj.targets:
target.settings = settings

plan = BakePlan.from_image_targets(config_obj.base_path, config_obj.targets, push_cache=True)

for bake_target in plan.target.values():
assert bake_target.cache_from is not None
assert bake_target.cache_to is not None
assert bake_target.cache_to[0]["ref"] == bake_target.cache_from[0]["ref"]
assert bake_target.cache_to[0]["mode"] == "max"

@pytest.mark.parametrize("suite", SUCCESS_SUITES)
def test_from_image_targets_with_temp_registry(self, get_expected_plan, get_config_obj, suite, resource_path):
"""Test that bake plans generate as expected with a temp registry."""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/scratch/cache:1.0.0-scratch"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/scratch/cache:1.0.0-scratch",
"mode": "max"
"ref": "ghcr.io/posit-dev/scratch/cache:1.0.0-scratch-amd64"
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-min"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-min",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-min-amd64"
}
]
},
Expand Down Expand Up @@ -111,14 +104,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-std"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-std",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-image/cache:1.0.0-ubuntu-22.04-std-amd64"
}
]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-min"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-min",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-min-amd64"
}
]
},
Expand Down Expand Up @@ -100,14 +93,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-min"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-min",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-min-amd64-arm64"
}
]
},
Expand Down Expand Up @@ -141,14 +127,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-std"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-std",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-22.04-std-amd64"
}
]
},
Expand Down Expand Up @@ -187,14 +166,7 @@
"cache_from": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-std"
}
],
"cache_to": [
{
"type": "registry",
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-std",
"mode": "max"
"ref": "ghcr.io/posit-dev/test-multi/cache:1.0.0-ubuntu-24.04-std-amd64-arm64"
}
]
}
Expand Down
10 changes: 7 additions & 3 deletions posit-bakery/test/image/test_image_target.py
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,10 @@ def test_build_args(self, basic_standard_image_target):
def test_build_args_cache_registry(self, basic_standard_image_target):
"""Test the build property of an ImageTarget."""
basic_standard_image_target.settings = ImageTargetSettings(cache_registry="ghcr.io/posit-dev")
# Cache name includes platform suffix
platforms = basic_standard_image_target.image_os.platforms
platform_suffix = "-".join(p.removeprefix("linux/").replace("/", "-") for p in platforms)
cache_name_with_platform = f"{basic_standard_image_target.cache_name}-{platform_suffix}"
expected_build_args = {
"context_path": basic_standard_image_target.context.base_path,
"file": basic_standard_image_target.containerfile,
Expand All @@ -407,10 +411,10 @@ def test_build_args_cache_registry(self, basic_standard_image_target):
"push": False,
"output": {},
"cache": True,
"cache_from": f"type=registry,ref={basic_standard_image_target.cache_name}",
"cache_to": f"type=registry,ref={basic_standard_image_target.cache_name},mode=max",
"cache_from": f"type=registry,ref={cache_name_with_platform}",
"cache_to": f"type=registry,ref={cache_name_with_platform},mode=max",
"metadata_file": None,
"platforms": ["linux/amd64"],
"platforms": platforms,
}

with patch("python_on_whales.docker.build") as mock_build:
Expand Down