push: Don't default to `DOCKER_DEFAULT_PLATFORM`, improve message #5246

vvoland · 2024-07-08T13:01:02Z

fixes: Clarify how to resolve pushing a single platform from a multi-platform image index #5242

Don't default the value of --platform flag to DOCKER_DEFAULT_PLATFORM environment variable and adjust the note.

- Description for the changelog

containerd integration: Fix `docker push` defaulting the `--platform` flag to a value of `DOCKER_DEFAULT_PLATFORM` environment variable on unsupported API versions.

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2024-07-08T13:08:24Z

Codecov Report

Attention: Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.

Project coverage is 61.46%. Comparing base (9bb1a62) to head (6c04adc).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5246      +/-   ##
==========================================
- Coverage   61.48%   61.46%   -0.02%     
==========================================
  Files         298      298              
  Lines       20811    20820       +9     
==========================================
+ Hits        12795    12798       +3     
- Misses       7103     7110       +7     
+ Partials      913      912       -1

krissetto · 2024-07-08T13:33:23Z

cli/command/image/push.go

@@ -81,7 +88,7 @@ func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error

 		printNote(dockerCli, `Selecting a single platform will only push one matching image manifest from a multi-platform image index.
 This means that any other components attached to the multi-platform image index (like Buildkit attestations) won't be pushed.
-If you want to only push a single platform image while preserving the attestations, please use 'docker convert\n'
+If you want to push a whole multi-platform image, make sure all image content is present and remove the --platform flag.


nit: i'd remove whole here.

Also, what does make sure all image content is present mean in this scenario? it's a bit confusing to me

You can have a multi-platform image, but only have partial content available.
For example, busybox is a multi-platform image, but doing docker pull busybox will only fetch the content for your daemon's host platform, assuming that the host is linux/amd64:

$ docker pull busybox $ docker pull --platform linux/arm64 busybox

Will pull the content for linux/amd64 and linux/arm64, but the "full" busybox image index still references all the other supported platforms (s390x, riscv64, arm/v5, ...).

Retagging the busybox as-is and attempting to push it to a different repository in most cases will fail, because the index will reference unknown blobs (of other platforms).

bit of an aside, but this NOTE color really doesn't look good in my terminal

I don't really understand what this note is trying to convey. Is it supposed to be a warning? When I read this it feels like I am doing something wrong. But we're not exactly saying that.
Can we clarify somehow?

You can have a multi-platform image, but only have partial content available.

For example, busybox is a multi-platform image, but doing docker pull busybox will only fetch the content for your daemon's host platform, assuming that the host is linux/amd64:

$ docker pull busybox $ docker pull --platform linux/arm64 busybox

Will pull the content for linux/amd64 and linux/arm64, but the "full" busybox image index still references all the other supported platforms (s390x, riscv64, arm/v5, ...).

Retagging the busybox as-is and attempting to push it to a different repository in most cases will fail, because the index will reference unknown blobs (of other platforms).

Thanks for that explanation @vvoland, makes sense.
I'm not convinced the terminology we're using is the clearest for the users though (when i hear image content i do not think about architectural variants, but the actual contents of the image - files etc.), but i can't think of many suggestions right now. Where else in the docs do we speak of these things? Do we use "content" with the same meaning in those places?

Applied some suggestions from @dvdksn and @jalonsogo.
Ran it through a bunch of different themes:

a.mp4

cli/command/image/push.go

Signed-off-by: Paweł Gronowski <[email protected]>

It's not merged yet. Signed-off-by: Paweł Gronowski <[email protected]>

dvdksn

LGTM

thaJeztah · 2024-07-09T19:04:27Z

cli/command/image/push.go

 		`Push a platform-specific manifest as a single-platform image to the registry.
+Image index won't be pushed, meaning that other manifests, including attestations will be lost.


The only part here that could still be ambiguous is "will be lost"; or at least; it will be still there, but won't be pushed.

I guess we can still tweak in follow-ups

Changed will be lost to won't be preserved, WDYT?

Benehiko

LGTM

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland · 2024-07-11T10:05:23Z

Thanks!

vvoland added process/cherry-pick kind/bugfix PR's that fix bugs area/ux containerd-integration Issues and PRs related to containerd integration process/cherry-pick/27.0 labels Jul 8, 2024

vvoland added this to the 28.0.0 milestone Jul 8, 2024

vvoland self-assigned this Jul 8, 2024

vvoland requested a review from a team July 8, 2024 13:05

vvoland force-pushed the push-nodefault-pre146 branch from 2a202d5 to 8e8a308 Compare July 8, 2024 13:05

krissetto reviewed Jul 8, 2024

View reviewed changes

thaJeztah reviewed Jul 8, 2024

View reviewed changes

cli/command/image/push.go Outdated Show resolved Hide resolved

vvoland force-pushed the push-nodefault-pre146 branch from 8e8a308 to 0915c28 Compare July 9, 2024 09:21

vvoland requested a review from thaJeztah July 9, 2024 09:21

vvoland force-pushed the push-nodefault-pre146 branch from 0915c28 to 363e037 Compare July 9, 2024 10:23

vvoland added 2 commits July 9, 2024 12:30

push: Don't default to DOCKER_DEFAULT_PLATFORM

4ce6e50

Signed-off-by: Paweł Gronowski <[email protected]>

c8d: Remove docker convert mention

d401994

It's not merged yet. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the push-nodefault-pre146 branch from 363e037 to d401994 Compare July 9, 2024 10:30

ESKYoung mentioned this pull request Jul 9, 2024

docker push fails with latest Docker Engine 27.0.3 due to dependency issues with Docker Engine API version abiosoft/colima#1063

Closed

5 tasks

vvoland force-pushed the push-nodefault-pre146 branch 4 times, most recently from e0ca02d to 9ae5a8e Compare July 9, 2024 15:22

vvoland requested a review from a team as a code owner July 9, 2024 15:22

vvoland changed the title ~~push: Don't default to DOCKER_DEFAULT_PLATFORM on older APIs~~ push: Don't default to DOCKER_DEFAULT_PLATFORM, improve message Jul 9, 2024

dvdksn reviewed Jul 9, 2024

View reviewed changes

vvoland requested a review from a team July 9, 2024 15:53

thaJeztah reviewed Jul 9, 2024

View reviewed changes

Benehiko approved these changes Jul 10, 2024

View reviewed changes

vvoland force-pushed the push-nodefault-pre146 branch from 9ae5a8e to 7320062 Compare July 10, 2024 09:22

push: Improve note message and colors

6c04adc

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the push-nodefault-pre146 branch from 7320062 to 6c04adc Compare July 10, 2024 09:36

vvoland requested a review from a team July 11, 2024 09:42

krissetto approved these changes Jul 11, 2024

View reviewed changes

vvoland merged commit 2f529a1 into docker:master Jul 11, 2024
88 checks passed

vvoland mentioned this pull request Jul 11, 2024

[27.1 backport] push: Don't default to DOCKER_DEFAULT_PLATFORM, improve message #5248

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick process/cherry-pick/27.0 labels Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

push: Don't default to `DOCKER_DEFAULT_PLATFORM`, improve message #5246

push: Don't default to `DOCKER_DEFAULT_PLATFORM`, improve message #5246

vvoland commented Jul 8, 2024 •

edited

Loading

codecov-commenter commented Jul 8, 2024 •

edited

Loading

krissetto Jul 8, 2024

vvoland Jul 8, 2024

dvdksn Jul 9, 2024

dvdksn Jul 9, 2024

krissetto Jul 9, 2024

vvoland Jul 9, 2024

dvdksn left a comment

thaJeztah Jul 9, 2024

vvoland Jul 10, 2024

Benehiko left a comment

vvoland commented Jul 11, 2024

		`Push a platform-specific manifest as a single-platform image to the registry.
		Image index won't be pushed, meaning that other manifests, including attestations will be lost.

push: Don't default to DOCKER_DEFAULT_PLATFORM, improve message #5246

push: Don't default to DOCKER_DEFAULT_PLATFORM, improve message #5246

Conversation

vvoland commented Jul 8, 2024 • edited Loading

codecov-commenter commented Jul 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdksn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

vvoland commented Jul 11, 2024

push: Don't default to `DOCKER_DEFAULT_PLATFORM`, improve message #5246

push: Don't default to `DOCKER_DEFAULT_PLATFORM`, improve message #5246

vvoland commented Jul 8, 2024 •

edited

Loading

codecov-commenter commented Jul 8, 2024 •

edited

Loading