-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
cli/push: Add platform
switch
#4984
Conversation
15cdbda
to
8e4f543
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4984 +/- ##
==========================================
- Coverage 61.83% 0 -61.84%
==========================================
Files 298 0 -298
Lines 20731 0 -20731
==========================================
- Hits 12818 0 -12818
+ Misses 7000 0 -7000
+ Partials 913 0 -913 |
cli/command/image/push.go
Outdated
@@ -84,6 +86,7 @@ func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error | |||
All: opts.all, | |||
RegistryAuth: encodedAuth, | |||
PrivilegeFunc: requestPrivilege, | |||
Platform: opts.platform, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warn that it will strip attestations (suggested by @thaJeztah)
8e4f543
to
53336d4
Compare
53336d4
to
827a96b
Compare
cli/command/image/push.go
Outdated
|
||
return cmd | ||
} | ||
|
||
// RunPush performs a push against the engine based on the specified options | ||
func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error { | ||
if opts.platform != "" { | ||
if _, isTty := term.GetFdInfo(dockerCli.Err()); isTty { | ||
_, _ = fmt.Fprint(dockerCli.Err(), "\x1b[1;37m\x1b[1;46m[ NOTE ]\x1b[0m\x1b[0m ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to repeat the reset here, only one is enough: \x1b[0m\x1b[0m
-> \x1b[0m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to add the cyan background color here? Why not \x1b[36m
to just color the text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch, no idea how that second one sneaked in there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting a colored background + colored text is safer – if we just set the foregroung color and the users color scheme uses the same color for the background (and I've definitely seen cyan backgrounds), then the text will be unreadable. If we set a contrasting background+foreground color combination then that's less likely to happen.
cli/command/image/push.go
Outdated
_, _ = fmt.Fprintln(dockerCli.Err(), `Selecting a single platform for the push operation will push the image manifest for that platform only. | ||
This won't push the image index/manifest list which means that other components like Buildkit attestations won't be pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about the wording here, we are talking about image manifests
, index
, manifest list
, do we really need to be this specific? Shouldn't we have a more user-friendly terms like "single-platform image" and "multi-platform image"?
No one really wants to know the difference between an index and a manifest list :)
cli/command/image/push.go
Outdated
} | ||
_, _ = fmt.Fprintln(dockerCli.Err(), `Selecting a single platform for the push operation will push the image manifest for that platform only. | ||
This won't push the image index/manifest list which means that other components like Buildkit attestations won't be pushed. | ||
If you want to only push a single platform while preserving the attestations, please build an image with only that platform and push it instead.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the above, after talking about manifests manifest lists and indexes we say "single platform"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that "single platform" isn't always a 1:1 mapping between an index and a single manifest.
Is the index below a single platform image or a multi-platform image?
{
"schemaVersion": 2,
"mediaType": "application/vnd.oci.image.index.v1+json",
"manifests": [
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"digest": "sha256:34b7d4a2f050f8a9077fd435b3b1778e091af743f0f4c8c47d109cfda47b0c48",
"size": 480,
"platform": {
"architecture": "arm64",
"os": "linux"
}
},
{
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"digest": "sha256:a7931924962b793438f641b320d496ebca34968e850f7b8d7a5ea59dc88283cc",
"size": 565,
"annotations": {
"vnd.docker.reference.digest": "sha256:34b7d4a2f050f8a9077fd435b3b1778e091af743f0f4c8c47d109cfda47b0c48",
"vnd.docker.reference.type": "attestation-manifest"
},
"platform": {
"architecture": "unknown",
"os": "unknown"
}
}
]
}
827a96b
to
0d0d8d5
Compare
13ffd29
to
cd1d450
Compare
5c8d268
to
b2de358
Compare
b2de358
to
dd83d4f
Compare
c1eef29
to
4f59b4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think we can discuss/keep improving the messages in the future too, but I wouldn't block this on that.
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' | ||
`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to be explicit here, I'd rather some users get annoyed by the extra message than having people not realize we're stripping attestations when they push a single-platform image.
4f59b4a
to
d6533a7
Compare
moby/moby#47943 was merged |
d6533a7
to
253077b
Compare
@krissetto @Benehiko can you TAL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
var notes []string | ||
|
||
func handleAux(dockerCli command.Cli) func(jm jsonmessage.JSONMessage) { | ||
return func(jm jsonmessage.JSONMessage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if none of data could be unmarshalled? should it not error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be other aux progress messages that are not (yet) supported by the CLI. I think it's best to just skip them instead.
253077b
to
431c06f
Compare
|
||
return cmd | ||
} | ||
|
||
// RunPush performs a push against the engine based on the specified options | ||
func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error { | ||
var platform *platforms.Platform | ||
if opts.platform != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, and not urgent, but if we decide to add more locations to pass --platform
, and we want to consume those as a platforms, we could consider implementing a platform _option_ that can be used for flags, performs the validation as part of that, and directly setting a
Platform`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should, but left that to a second PR that will reuse this logic.
Btw, we already have it, but for the "string" platform:
Line 158 in 0022fe7
flags.StringVar(target, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Set platform if server is multi-platform capable") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! We should look where/how that's used. If the result is only "internal", it could still make sense to change it, and have the consumer convert it to a string (where needed 🤔).
In either case; all for separate work, just that I thought of it.
cli/command/image/push.go
Outdated
} | ||
|
||
func printNote(dockerCli command.Cli, format string, args ...any) { | ||
if _, isTty := term.GetFdInfo(dockerCli.Err()); isTty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that dockerCli.Out()
has a IsTerminal()
function that we can use. Not sure if that's fully correct though (is that correct if STDERR
redirected, but STDOUT
isn't?
We should consider providing this for both Out()
and Err()
(I looked at that a few times, and wondered why we didn't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most common case of this is when you redirect the stderr to /dev/null
:
$ docker push dckr.woland.xyz/ububuu
Using default tag: latest
The push refers to repository [dckr.woland.xyz/ububuu]
aa21f24e1940: Layer already exists
latest: digest: sha256:17c24d16d63d2d089db74c2ed3e99c1ab0fd3f4f93c00b04afa8793fa793626c size: 424
[ NOTE ] Not all multiplatform-content is present and only the available single-platform image was pushed
sha256:e3f92abc0967a6c19d0dfa2d55838833e947b9d74edbcb0113e48535ad4be12a -> sha256:17c24d16d63d2d089db74c2ed3e99c1ab0fd3f4f93c00b04afa8793fa793626c
$ docker push dckr.woland.xyz/ububuu 2>/dev/null
Using default tag: latest
The push refers to repository [dckr.woland.xyz/ububuu]
aa21f24e1940: Layer already exists
latest: digest: sha256:17c24d16d63d2d089db74c2ed3e99c1ab0fd3f4f93c00b04afa8793fa793626c size: 424
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think there's other places where we do it wrong, and only look at STDOUT
, but not at STDERR
(but switch both based on STDOUT
; here's a recent discussion that I still need to reply to, because I think the same is happening for docker build
; moby/moby#47755
Perhaps a good reason to look if we should make Err()
provide thee same features as Out()
, so that it's easier to check for each of the outputs if they have a TTY attached or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In either case; not a blocker (should I mention that isTty
should be isTTY
? 😂 🙈 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will open a follow-up with the stderr
change (it will have a bunch of updates in other places, so prefer to handle it in a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isTTY
LOL, you're just merciless Sebastiaan 🤣
...done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so prefer to handle it in a separate PR
Yes; that's for sure something separate; and it's not a new thing, just that this is (I think) the first time we pay attention and check the right thing for a TTY 😂
OL, you're just merciless Sebastiaan 🤣
I'm the Human Linter! 😂
431c06f
to
9e06e58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think most comments still visible are good for follow-up work.
full diff: moby/moby@a736d07...9d94884 Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Print note when the multi-platform image was reduced to a single manifest. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
9e06e58
to
32ac7a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Let's go!
--platform
switch moby/moby#47679- What I did
Added a
platform
switch todocker image push
.- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)