-
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
base: master
Are you sure you want to change the base?
Conversation
15cdbda
to
8e4f543
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4984 +/- ##
==========================================
- Coverage 61.32% 61.22% -0.10%
==========================================
Files 298 298
Lines 20694 20740 +46
==========================================
+ Hits 12691 12699 +8
- Misses 7102 7137 +35
- Partials 901 904 +3 |
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.
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
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
5c8d268
to
b2de358
Compare
--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)