Implement ability to load images by default in non-Docker build drivers. #2259

n-g · 2024-02-16T10:08:25Z

This eases build driver migrations, as it allows aligning the default behavior.

See also https://docs.docker.com/build/drivers/

tonistiigi

This could also be a driver-opt maybe?

controller/build/build.go

n-g · 2024-02-22T22:16:48Z

This could also be a driver-opt maybe?

Thanks! I like this path, as it allows finer granularity when selecting the new behavior (per driver vs per env).
From what I can tell, this is the first driver-opt applicable to multiple driver types.
I introduced a new feature, but lmk if there is a cleaner modeling path I could employ.

driver/docker-container/driver.go

driver/features.go

n-g · 2024-03-08T09:29:23Z

@tonistiigi @crazy-max
Thanks for taking a look at this!
From what I can tell, I addressed all your suggestions.
Is there anything left to do to make this PR ready to merge?
Thanks!

driver/features.go

crazy-max

Can you squash your commits please?

Also would be nice to have integration tests in https://github.com/docker/buildx/tree/master/tests if you can or can be follow-up

Thanks!

driver/features.go

n-g · 2024-03-08T15:44:07Z

Can you squash your commits please?

Also would be nice to have integration tests in https://github.com/docker/buildx/tree/master/tests if you can or can be follow-up

Thanks!

Sure!

I initially did not realize that not all workers are tested by default with ./hack/test.
Updated the test now, and validated it with
TESTFLAGS="-v --parallel=1 --timeout=30m --run=//worker=docker$" TEST_DOCKERD=1 TESTFLAGS_DOCKER="-v --parallel=1 --timeout=30m" ./hack/test locally.

PTAL

tests/build.go

n-g · 2024-03-15T16:03:10Z

@crazy-max Could you help me re-trigger the validation workflows?

n-g · 2024-03-18T15:09:53Z

@crazy-max Could you help me re-trigger the validation workflows?

Thanks!
Let me know if there is anything else I can help with here 🙏

crazy-max

LGTM thanks!

fix tests needed

crazy-max

It needs a rebase with changes introduced recently sorry 😅
See also review, I tried to push changes to your branch but looks like you don't allow edits by maintainers. Here is my branch if you want to pick these changes: master...crazy-max:buildx:default-load-opt otherwise I can carry in another PR if you prefer. Thanks

crazy-max · 2024-04-05T11:20:40Z

tests/build.go

+	if sb.Name() != "docker" {
+		t.Skip("skipping test for non-docker workers")
+	}


Use helper, see bcda1db

crazy-max · 2024-04-05T11:21:00Z

tests/build.go

+	registry, err := sb.NewRegistry()
+	if errors.Is(err, integration.ErrRequirements) {
+		t.Skip(err.Error())
+	}
+	require.NoError(t, err)
+
+	target := registry + "/buildx/registry:" + identity.NewID()


Setting up registry is not needed for this test, see 4d012cf

n-g · 2024-04-05T13:02:10Z

It needs a rebase with changes introduced recently sorry 😅 See also review, I tried to push changes to your branch but looks like you don't allow edits by maintainers. Here is my branch if you want to pick these changes: master...crazy-max:buildx:default-load-opt otherwise I can carry in another PR if you prefer. Thanks

Thanks!
Rebased and applied your suggestions.

From what I can tell, I cannot grant edit rights retroactively.
Feel free to ping me if you need any changes.

crazy-max · 2024-04-05T13:21:42Z

tests/build.go

+	cmd.Env = append(cmd.Env, "BUILDX_BUILDER="+builderName)
+	outb, err := cmd.CombinedOutput()
+	require.NoError(t, err, string(outb))
+}


The image inspect test seems to have been removed: master...crazy-max:buildx:default-load-opt#diff-12855e5a65cd0dc084324f4808d159cf2ebd514a77c6eee4dba653e1a4e94470R793-R795

Yup, sorry, bad merge. Was juggling too many things.
Updated and tested locally 🤞

This eases build driver migrations, as it allows aligning the default behavior. See also https://docs.docker.com/build/drivers/ Signed-off-by: Niklas Gehlen <niklas@namespacelabs.com>

crazy-max

Thanks!

n-g force-pushed the master branch from 36d3159 to d3c829c Compare February 16, 2024 10:10

tonistiigi reviewed Feb 22, 2024

View reviewed changes

controller/build/build.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch from d3c829c to de09615 Compare February 22, 2024 21:04

n-g requested a review from tonistiigi February 22, 2024 22:18

n-g force-pushed the master branch 2 times, most recently from 3ba8f17 to 9889528 Compare February 27, 2024 11:04

crazy-max reviewed Feb 28, 2024

View reviewed changes

driver/docker-container/driver.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch 2 times, most recently from f93683d to 03340c5 Compare February 29, 2024 10:41

n-g requested a review from crazy-max February 29, 2024 10:44

crazy-max reviewed Feb 29, 2024

View reviewed changes

driver/features.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch from f41091c to d6ab70c Compare February 29, 2024 11:10

n-g requested a review from crazy-max March 8, 2024 09:27

dvdksn reviewed Mar 8, 2024

View reviewed changes

driver/features.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch 2 times, most recently from fa960fa to 8e7dbc3 Compare March 8, 2024 11:37

n-g requested a review from dvdksn March 8, 2024 11:37

crazy-max reviewed Mar 8, 2024

View reviewed changes

driver/features.go Show resolved Hide resolved

n-g force-pushed the master branch from ca9ff9d to 2e9532e Compare March 8, 2024 15:02

crazy-max added the needs/docs-follow-up label Mar 8, 2024

n-g force-pushed the master branch from 2e9532e to b2d5e8d Compare March 8, 2024 15:32

n-g requested a review from crazy-max March 8, 2024 15:44

crazy-max requested changes Mar 8, 2024

View reviewed changes

tests/build.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch 2 times, most recently from 2fd4de2 to e8b5973 Compare March 8, 2024 16:22

n-g requested a review from crazy-max March 8, 2024 16:23

crazy-max reviewed Mar 8, 2024

View reviewed changes

tests/build.go Outdated Show resolved Hide resolved

crazy-max reviewed Mar 8, 2024

View reviewed changes

tests/build.go Outdated Show resolved Hide resolved

n-g force-pushed the master branch from e8b5973 to f992dfe Compare March 8, 2024 16:52

n-g requested a review from crazy-max March 8, 2024 16:52

n-g force-pushed the master branch from f992dfe to 55c766c Compare March 15, 2024 15:47

dvdksn mentioned this pull request Mar 15, 2024

buildx: default-load driver opt docker/docs#19640

Merged

crazy-max previously approved these changes Mar 28, 2024

View reviewed changes

crazy-max added this to the v0.14.0 milestone Mar 28, 2024

n-g force-pushed the master branch from 55c766c to 86a937c Compare March 28, 2024 09:55

dvdksn removed the needs/docs-follow-up label Apr 3, 2024

crazy-max requested changes Apr 5, 2024

View reviewed changes

n-g force-pushed the master branch 3 times, most recently from c7012c1 to 58a45d9 Compare April 5, 2024 12:47

n-g force-pushed the master branch from 58a45d9 to c374f43 Compare April 5, 2024 13:19

crazy-max reviewed Apr 5, 2024

View reviewed changes

Implement new driver-opt: default-load

ccc314a

This eases build driver migrations, as it allows aligning the default behavior. See also https://docs.docker.com/build/drivers/ Signed-off-by: Niklas Gehlen <niklas@namespacelabs.com>

n-g force-pushed the master branch from c374f43 to ccc314a Compare April 5, 2024 13:32

crazy-max approved these changes Apr 5, 2024

View reviewed changes

crazy-max merged commit 4b3c3c8 into docker:master Apr 5, 2024
67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ability to load images by default in non-Docker build drivers. #2259

Implement ability to load images by default in non-Docker build drivers. #2259

n-g commented Feb 16, 2024

tonistiigi left a comment

n-g commented Feb 22, 2024

n-g commented Mar 8, 2024

crazy-max left a comment

n-g commented Mar 8, 2024

n-g commented Mar 15, 2024

n-g commented Mar 18, 2024

crazy-max left a comment

crazy-max left a comment

crazy-max Apr 5, 2024

crazy-max Apr 5, 2024 •

edited

n-g commented Apr 5, 2024

crazy-max Apr 5, 2024

n-g Apr 5, 2024

crazy-max left a comment

Implement ability to load images by default in non-Docker build drivers. #2259

Implement ability to load images by default in non-Docker build drivers. #2259

Conversation

n-g commented Feb 16, 2024

tonistiigi left a comment

Choose a reason for hiding this comment

n-g commented Feb 22, 2024

n-g commented Mar 8, 2024

crazy-max left a comment

Choose a reason for hiding this comment

n-g commented Mar 8, 2024

n-g commented Mar 15, 2024

n-g commented Mar 18, 2024

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max Apr 5, 2024

Choose a reason for hiding this comment

crazy-max Apr 5, 2024 • edited

Choose a reason for hiding this comment

n-g commented Apr 5, 2024

crazy-max Apr 5, 2024

Choose a reason for hiding this comment

n-g Apr 5, 2024

Choose a reason for hiding this comment

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max Apr 5, 2024 •

edited