Skip to content
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

Support for docker compose build --push #10148

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

maxcleme
Copy link
Member

@maxcleme maxcleme commented Jan 6, 2023

Signed-off-by: maxcleme [email protected]

What I did

Support docker compose build --push the same way as docker buildx build --push does.

Oddly, I was expecting it to be already supported. It occurs to me when dealing with a simple compose file locally using two platforms (linux/arm64 & linux/amd64).

However when doing docker compose push it only pushes the local image (which makes sense since my store can't really store the other). Going through the same as docker buildx build does seems logical here.

This PR is a bit opportunistic, all feedbacks are welcome 😄

Related issue

@maxcleme maxcleme force-pushed the feat/support_multiarch_push branch from 113ba24 to d2b8da7 Compare January 9, 2023 09:42
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Base: 72.79% // Head: 73.89% // Increases project coverage by +1.10% 🎉

Coverage data is based on head (e05e604) compared to base (cf12239).
Patch has no changes to coverable lines.

❗ Current head e05e604 differs from pull request most recent head 634a7d2. Consider uploading reports for the commit 634a7d2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10148      +/-   ##
==========================================
+ Coverage   72.79%   73.89%   +1.10%     
==========================================
  Files           2        2              
  Lines         272      272              
==========================================
+ Hits          198      201       +3     
+ Misses         62       60       -2     
+ Partials       12       11       -1     
Impacted Files Coverage Δ
pkg/e2e/framework.go 72.15% <0.00%> (+1.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM but for this option to be introduced, the "classic" builder also need to offer this feature

@maxcleme
Copy link
Member Author

maxcleme commented Jan 9, 2023

@ndeloof What do you mean by "classic builder" ? Do you mind sharing more pointers? I'll be glad to update accordingly :)

@ndeloof
Copy link
Contributor

ndeloof commented Jan 9, 2023

in this PR you only updated the options, but the actual build implementation is here:
https://github.com/docker/compose/blob/v2/pkg/compose/build_buildkit.go
https://github.com/docker/compose/blob/v2/pkg/compose/build_classic.go

first one uses buildkit (same as docker buildx), the other uses the classic build mechanism on docker engine

@maxcleme maxcleme force-pushed the feat/support_multiarch_push branch from d2b8da7 to be779c0 Compare January 9, 2023 16:08
pkg/compose/build.go Outdated Show resolved Hide resolved
@maxcleme maxcleme requested a review from ndeloof January 9, 2023 16:09
@maxcleme maxcleme force-pushed the feat/support_multiarch_push branch from be779c0 to 4a224a8 Compare January 10, 2023 08:39
@maxcleme maxcleme changed the title Support for docker compose build --push when using multiple platforms Support for docker compose build --push Jan 10, 2023
@maxcleme
Copy link
Member Author

Any news on this? 🤔

@glours
Copy link
Contributor

glours commented Jan 24, 2023

I did some tests, it works well except for one use case. When you try to build and push an multi-arch image with the legacy builder we got the following error message :

~/sources/compose/pkg/e2e/fixtures/build-test/platforms feat/support_multiarch_push !1 > DOCKER_BUILDKIT=0 docker compose build --push                                                                 19:02:19
An image does not exist locally with the tag: gloursdocker/build-test-platform

instead of the message saying that the builder doesn't support the multi-arch builds

~/sources/compose/pkg/e2e/fixtures/build-test/platforms feat/support_multiarch_push !1 > DOCKER_BUILDKIT=0 docker compose build                                                                        19:02:46
1 error occurred:
	* this builder doesn't support multi-arch build, set DOCKER_BUILDKIT=1 to use multi-arch builder

@glours glours self-assigned this Jan 25, 2023
@maxcleme maxcleme force-pushed the feat/support_multiarch_push branch from 4a224a8 to e05e604 Compare January 26, 2023 07:46
@maxcleme
Copy link
Member Author

maxcleme commented Jan 26, 2023

@glours Nice catch indeed! Should be fixed now, do you mind giving it a try again? :)

@glours
Copy link
Contributor

glours commented Jan 26, 2023

Looks good 👍

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours force-pushed the feat/support_multiarch_push branch from e05e604 to 634a7d2 Compare January 26, 2023 15:54
if errs != nil {
return nil
}
if len(o.Exports) != 0 && o.Exports[0].Attrs["push"] == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

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

this really demonstrates we should not use build.Options as input for classic builder, but that's fine for this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, I'll try to find time to review that

@glours glours enabled auto-merge January 26, 2023 16:01
@glours glours merged commit 8bb9a33 into docker:v2 Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants