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.

Sign up for GitHub

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

Jump to bottom

cli/push: Add platform switch #4984

Merged
merged 3 commits into from
Jun 11, 2024
Merged

cli/push: Add platform switch #4984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
892784d
vendor: github.com/docker/docker v27.0.0-dev (9d9488468fe2)
vvoland Jun 11, 2024
966fa7c
cli/push: Add `platform` switch
vvoland Apr 3, 2024
32ac7a0
cli/push: Print aux notes
vvoland Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 79 additions & 2 deletions cli/command/image/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,25 @@ package image

import (
"context"
"encoding/json"
"fmt"
"io"
"os"

"github.com/containerd/platforms"
"github.com/distribution/reference"
"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/completion"
"github.com/docker/cli/cli/streams"
"github.com/docker/docker/api/types/auxprogress"
"github.com/docker/docker/api/types/image"
registrytypes "github.com/docker/docker/api/types/registry"
"github.com/docker/docker/pkg/jsonmessage"
"github.com/docker/docker/registry"
"github.com/moby/term"
"github.com/morikuni/aec"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)
Expand All @@ -23,6 +30,7 @@ type pushOptions struct {
remote string
untrusted bool
quiet bool
platform string
}

// NewPushCommand creates a new `docker push` command
Expand All @@ -48,12 +56,33 @@ func NewPushCommand(dockerCli command.Cli) *cobra.Command {
flags.BoolVarP(&opts.all, "all-tags", "a", false, "Push all tags of an image to the repository")
flags.BoolVarP(&opts.quiet, "quiet", "q", false, "Suppress verbose output")
command.AddTrustSigningFlags(flags, &opts.untrusted, dockerCli.ContentTrustEnabled())
flags.StringVar(&opts.platform, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"),
`Push a platform-specific manifest as a single-platform image to the registry.
'os[/arch[/variant]]': Explicit platform (eg. linux/amd64)`)
flags.SetAnnotation("platform", "version", []string{"1.46"})

return cmd
}

// RunPush performs a push against the engine based on the specified options
//
//nolint:gocyclo
func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error {
var platform *ocispec.Platform
if opts.platform != "" {
Copy link
Member

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`.

Copy link
Collaborator Author

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:

cli/cli/command/utils.go

Line 158 in 0022fe7

flags.StringVar(target, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Set platform if server is multi-platform capable")
🙈

Copy link
Member

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.

p, err := platforms.Parse(opts.platform)
if err != nil {
_, _ = fmt.Fprintf(dockerCli.Err(), "Invalid platform %s", opts.platform)
return err
}
platform = &p

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'
`)
Comment on lines +80 to +83
Copy link
Member

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.

}

ref, err := reference.ParseNormalizedNamed(opts.remote)
switch {
case err != nil:
Expand Down Expand Up @@ -84,25 +113,73 @@ func RunPush(ctx context.Context, dockerCli command.Cli, opts pushOptions) error
All: opts.all,
RegistryAuth: encodedAuth,
PrivilegeFunc: requestPrivilege,
Platform: platform,
}

responseBody, err := dockerCli.Client().ImagePush(ctx, reference.FamiliarString(ref), options)
if err != nil {
return err
}

defer func() {
for _, note := range notes {
fmt.Fprintln(dockerCli.Err(), "")
printNote(dockerCli, note)
}
}()

defer responseBody.Close()
if !opts.untrusted {
// TODO PushTrustedReference currently doesn't respect `--quiet`
return PushTrustedReference(dockerCli, repoInfo, ref, authConfig, responseBody)
}

if opts.quiet {
err = jsonmessage.DisplayJSONMessagesToStream(responseBody, streams.NewOut(io.Discard), nil)
err = jsonmessage.DisplayJSONMessagesToStream(responseBody, streams.NewOut(io.Discard), handleAux(dockerCli))
if err == nil {
fmt.Fprintln(dockerCli.Out(), ref.String())
}
return err
}
return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil)
return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), handleAux(dockerCli))
}

var notes []string

func handleAux(dockerCli command.Cli) func(jm jsonmessage.JSONMessage) {
return func(jm jsonmessage.JSONMessage) {
Copy link
Member

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?

Copy link
Collaborator Author

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.

b := []byte(*jm.Aux)

var stripped auxprogress.ManifestPushedInsteadOfIndex
err := json.Unmarshal(b, &stripped)
if err == nil && stripped.ManifestPushedInsteadOfIndex {
note := fmt.Sprintf("Not all multiplatform-content is present and only the available single-platform image was pushed\n%s -> %s",
aec.RedF.Apply(stripped.OriginalIndex.Digest.String()),
aec.GreenF.Apply(stripped.SelectedManifest.Digest.String()),
)
notes = append(notes, note)
}

var missing auxprogress.ContentMissing
err = json.Unmarshal(b, &missing)
if err == nil && missing.ContentMissing {
note := `You're trying to push a manifest list/index which
references multiple platform specific manifests, but not all of them are available locally
or available to the remote repository.

Make sure you have all the referenced content and try again.

You can also push only a single platform specific manifest directly by specifying the platform you want to push with the --platform flag.`
notes = append(notes, note)
}
}
}

func printNote(dockerCli command.Cli, format string, args ...any) {
if _, isTTY := term.GetFdInfo(dockerCli.Err()); isTTY {
_, _ = fmt.Fprint(dockerCli.Err(), aec.WhiteF.Apply(aec.CyanB.Apply("[ NOTE ]"))+" ")
} else {
_, _ = fmt.Fprint(dockerCli.Err(), "[ NOTE ] ")
}
_, _ = fmt.Fprintf(dockerCli.Err(), aec.Bold.Apply(format)+"\n", args...)
}
11 changes: 6 additions & 5 deletions docs/reference/commandline/image_push.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ Upload an image to a registry

### Options

| Name | Type | Default | Description |
|:---------------------------------------------|:-------|:--------|:--------------------------------------------|
| [`-a`](#all-tags), [`--all-tags`](#all-tags) | | | Push all tags of an image to the repository |
| `--disable-content-trust` | `bool` | `true` | Skip image signing |
| `-q`, `--quiet` | | | Suppress verbose output |
| Name | Type | Default | Description |
|:---------------------------------------------|:---------|:--------|:--------------------------------------------------------------------------------------------------------------------------------------------|
| [`-a`](#all-tags), [`--all-tags`](#all-tags) | | | Push all tags of an image to the repository |
| `--disable-content-trust` | `bool` | `true` | Skip image signing |
| `--platform` | `string` | | Push a platform-specific manifest as a single-platform image to the registry.<br>'os[/arch[/variant]]': Explicit platform (eg. linux/amd64) |
| `-q`, `--quiet` | | | Suppress verbose output |


<!---MARKER_GEN_END-->
Expand Down
11 changes: 6 additions & 5 deletions docs/reference/commandline/push.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ Upload an image to a registry

### Options

| Name | Type | Default | Description |
|:--------------------------|:-------|:--------|:--------------------------------------------|
| `-a`, `--all-tags` | | | Push all tags of an image to the repository |
| `--disable-content-trust` | `bool` | `true` | Skip image signing |
| `-q`, `--quiet` | | | Suppress verbose output |
| Name | Type | Default | Description |
|:--------------------------|:---------|:--------|:--------------------------------------------------------------------------------------------------------------------------------------------|
| `-a`, `--all-tags` | | | Push all tags of an image to the repository |
| `--disable-content-trust` | `bool` | `true` | Skip image signing |
| `--platform` | `string` | | Push a platform-specific manifest as a single-platform image to the registry.<br>'os[/arch[/variant]]': Explicit platform (eg. linux/amd64) |
| `-q`, `--quiet` | | | Suppress verbose output |


<!---MARKER_GEN_END-->
Expand Down
2 changes: 1 addition & 1 deletion vendor.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/creack/pty v1.1.21
github.com/distribution/reference v0.6.0
github.com/docker/distribution v2.8.3+incompatible
github.com/docker/docker v26.1.1-0.20240610145149-a736d0701c41+incompatible // master (v27.0.0-dev)
github.com/docker/docker v26.1.1-0.20240610201418-9d9488468fe2+incompatible // master (v27.0.0-dev)
github.com/docker/docker-credential-helpers v0.8.2
github.com/docker/go-connections v0.5.0
github.com/docker/go-units v0.5.0
Expand Down
4 changes: 2 additions & 2 deletions vendor.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5
github.com/docker/distribution v2.7.1+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
github.com/docker/distribution v2.8.3+incompatible h1:AtKxIZ36LoNK51+Z6RpzLpddBirtxJnzDrHLEKxTAYk=
github.com/docker/distribution v2.8.3+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
github.com/docker/docker v26.1.1-0.20240610145149-a736d0701c41+incompatible h1:Kraon288jb3POkrmM5w6Xo979z2rrCtFzHycAjafRes=
github.com/docker/docker v26.1.1-0.20240610145149-a736d0701c41+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
github.com/docker/docker v26.1.1-0.20240610201418-9d9488468fe2+incompatible h1:k63BdhjySkwvmdeofOsBElcuVrWaDBrI7FQgnyoVnnM=
github.com/docker/docker v26.1.1-0.20240610201418-9d9488468fe2+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
github.com/docker/docker-credential-helpers v0.8.2 h1:bX3YxiGzFP5sOXWc3bTPEXdEaZSeVMrFgOr3T+zrFAo=
github.com/docker/docker-credential-helpers v0.8.2/go.mod h1:P3ci7E3lwkZg6XiHdRKft1KckHiO9a2rNtyFbZ/ry9M=
github.com/docker/go v1.5.1-1.0.20160303222718-d30aec9fd63c h1:lzqkGL9b3znc+ZUgi7FlLnqjQhcXxkNM/quxIjBVMD0=
Expand Down
42 changes: 30 additions & 12 deletions vendor/github.com/docker/docker/api/swagger.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions vendor/github.com/docker/docker/api/types/auxprogress/push.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 18 additions & 1 deletion vendor/github.com/docker/docker/api/types/image/opts.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading