Add docker image output field to support publish to repository when using BuildKit #20154

riisi · 2023-11-07T19:03:59Z

Currently, the publish goal doesn't work with docker images when buildkit is enabled, as by default buildkit doesn't save the build output locally, and publish expects that the images were saved.

This PR adds support for setting the output type, and defaults it todocker, which is the legacy docker build behavior, i.e. saves to the local image store.

However, we only want to set that when buildkit is enabled. I thought it better to add an explicit option for that at the subsystem level; this allows for validation of buildkit-only options.

This eliminates the need to set DOCKER_BUILDKIT=1 in env vars - I need to update the docs on that actually.

I have validated that with this change, docker images can be published to a registry.

kaos

Very nice. This will make it much easier to get into build kit mode :)

src/python/pants/backend/docker/goals/package_image.py

src/python/pants/backend/docker/target_types.py

kaos

We're getting closer :)

src/python/pants/backend/docker/target_types.py

kaos · 2023-11-08T20:04:30Z

src/python/pants/backend/docker/goals/package_image.py

-                error_cls=DockerImageOptionValueError,
-            )
-            yield from target[field_type].options(format)
+        elif issubclass(field_type, DockerBuildKitOptionField):


sorry, but I have an itch to scratch here. I think I may have an idea how to further improve this logic slightly.

There are two things going on here now, one is validating option use and the other is to produce the option values themselves.

Looking at this elif chain, I fear it'll be too easy to add a new option that inherits from both DockerBuildOptionFieldMixin and DockerBuildKitOptionField and then you don't get the behaviour we want.

How about splitting this into two elif chains. The first for option validation (such as the use buildx toggle for buildkit options) and the other for producing the option values. That would also get rid of the duplication we have now.

Seems a lot cleaner now - let me know

riisi · 2023-11-09T15:51:14Z

src/python/pants/backend/docker/goals/package_image.py

@@ -347,7 +350,7 @@ def get_build_options(
                source=source,
                error_cls=DockerImageOptionValueError,
            )
-            yield from target[field_type].options(format)
+            yield from target[field_type].options(format, global_build_hosts_options=global_build_hosts_options)  # type: ignore[attr-defined]


I assume this is a mypy bug but couldn't find any workaround. It seems to occur when there are more than 2 subclasses added to issubclass- e.g. splitting into separate if statements for each subclass doesn't trigger it.

interesting. we can go with this for now.. I'll have a look on my end, if I'll find anything I can follow up in a new PR in that case.

kaos · 2023-11-13T15:21:43Z

src/python/pants/backend/docker/goals/package_image.py

@@ -347,7 +350,7 @@ def get_build_options(
                source=source,
                error_cls=DockerImageOptionValueError,
            )
-            yield from target[field_type].options(format)
+            yield from target[field_type].options(format, global_build_hosts_options=global_build_hosts_options)  # type: ignore[attr-defined]


interesting. we can go with this for now.. I'll have a look on my end, if I'll find anything I can follow up in a new PR in that case.

…sing BuildKit (#20154) Currently, the `publish` goal doesn't work with docker images when buildkit is enabled, as by [default buildkit doesn't save the build output locally](docker/buildx#166), and `publish` expects that the images were saved. This PR adds support for setting the output type, and defaults it to`docker`, which is the legacy docker build behavior, i.e. saves to the local image store. However, we only want to set that when buildkit is enabled. I thought it better to add an explicit option for that at the subsystem level; this allows for validation of buildkit-only options. This eliminates the need to set `DOCKER_BUILDKIT=1` in env vars - I need to update the docs on that actually. I have validated that with this change, docker images can be published to a registry. --------- Co-authored-by: Rhys Madigan <rhys.madigan@accenture.com>

WorkerPants · 2023-11-15T00:43:23Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.19.x

Successfully opened #20185.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

WorkerPants · 2023-11-15T05:51:57Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.18.x

I was unable to cherry-pick this PR to 2.18.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.18.x \
  && git checkout -b cherry-pick-20154-to-2.18.x FETCH_HEAD \
  && git cherry-pick c621a7145ad88543ab61dc3336b91a05f5ec86cf

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "20154" "2.18.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

✔️ 2.19.x

Successfully opened #20185.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

…sing BuildKit (Cherry-pick of #20154) (#20185) Currently, the `publish` goal doesn't work with docker images when buildkit is enabled, as by [default buildkit doesn't save the build output locally](docker/buildx#166), and `publish` expects that the images were saved. This PR adds support for setting the output type, and defaults it to`docker`, which is the legacy docker build behavior, i.e. saves to the local image store. However, we only want to set that when buildkit is enabled. I thought it better to add an explicit option for that at the subsystem level; this allows for validation of buildkit-only options. This eliminates the need to set `DOCKER_BUILDKIT=1` in env vars - I need to update the docs on that actually. I have validated that with this change, docker images can be published to a registry. Co-authored-by: riisi <rhysmadigan@gmail.com> Co-authored-by: Rhys Madigan <rhys.madigan@accenture.com>

kaos · 2023-11-15T18:39:05Z

@huonw apologies, my bad I got this feature mixed up with something else, so we only need it picked to 2.19.

Rhys Madigan added 3 commits November 7, 2023 18:13

initial add

5f24c14

Flake8 fixes

086f140

Fix build command

b4be23a

riisi added the backend: Docker Docker backend-related issues label Nov 7, 2023

riisi requested a review from kaos November 7, 2023 19:04

Merge branch 'main' into build-output

1689874

kaos reviewed Nov 7, 2023

View reviewed changes

src/python/pants/backend/docker/goals/package_image.py Outdated Show resolved Hide resolved

src/python/pants/backend/docker/target_types.py Outdated Show resolved Hide resolved

src/python/pants/backend/docker/target_types.py Outdated Show resolved Hide resolved

kaos added the category:new feature label Nov 7, 2023

PR fixes

668f062

riisi commented Nov 8, 2023

View reviewed changes

src/python/pants/backend/docker/target_types.py Show resolved Hide resolved

kaos reviewed Nov 8, 2023

View reviewed changes

Refactor options subclass handling

3ca7468

riisi commented Nov 9, 2023

View reviewed changes

kaos approved these changes Nov 13, 2023

View reviewed changes

kaos changed the title ~~Add docker image output field to support publish to repository~~ Add docker image output field to support publish to repository when using BuildKit Nov 13, 2023

kaos merged commit c621a71 into pantsbuild:main Nov 13, 2023

huonw added the needs-cherrypick label Nov 15, 2023

huonw added this to the 2.19.x milestone Nov 15, 2023

WorkerPants removed the needs-cherrypick label Nov 15, 2023

huonw modified the milestones: 2.19.x, 2.18.x Nov 15, 2023

huonw added the needs-cherrypick label Nov 15, 2023

WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Nov 15, 2023

kaos modified the milestones: 2.18.x, 2.19.x Nov 15, 2023

kaos removed the needs-cherrypick label Nov 15, 2023

kaos removed the auto-cherry-picking-failed Auto Cherry-Picking Failed label Nov 15, 2023

kaos mentioned this pull request Nov 15, 2023

docker backend doesn't use buildx for multi-platform #19994

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docker image output field to support publish to repository when using BuildKit #20154

Add docker image output field to support publish to repository when using BuildKit #20154

riisi commented Nov 7, 2023

kaos left a comment

kaos left a comment

kaos Nov 8, 2023

riisi Nov 9, 2023

riisi Nov 9, 2023

kaos Nov 13, 2023

kaos Nov 13, 2023

WorkerPants commented Nov 15, 2023

WorkerPants commented Nov 15, 2023

kaos commented Nov 15, 2023

Add docker image output field to support publish to repository when using BuildKit #20154

Add docker image output field to support publish to repository when using BuildKit #20154

Conversation

riisi commented Nov 7, 2023

kaos left a comment

Choose a reason for hiding this comment

kaos left a comment

Choose a reason for hiding this comment

kaos Nov 8, 2023

Choose a reason for hiding this comment

riisi Nov 9, 2023

Choose a reason for hiding this comment

riisi Nov 9, 2023

Choose a reason for hiding this comment

kaos Nov 13, 2023

Choose a reason for hiding this comment

kaos Nov 13, 2023

Choose a reason for hiding this comment

WorkerPants commented Nov 15, 2023

✔️ 2.19.x

WorkerPants commented Nov 15, 2023

❌ 2.18.x

✔️ 2.19.x

kaos commented Nov 15, 2023