From d135dc242e0e138587292bfb6cc28a1899e06a96 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Tue, 10 Dec 2019 12:24:14 +0100
Subject: [PATCH] client.ImagePush(): default to ":latest" instead of "all
 tags"

The `docker push` command up until docker v0.9.1 always pushed all tags of a given
image, so `docker push foo/bar` would push (e.g.) all of  `foo/bar:latest`, `foo:/bar:v1`,
and `foo/bar:v1.0.0`.

Pushing all tags of an image was not desirable in many case, so docker v0.10.0
enhanced `docker push` to optionally specify a tag to push (`docker push foo/bar:v1`)
(see issue 3411 and PR 4948 (commit e648a186d68dcb3ee0d6123b041c5aa66438cc89).

This behavior exists up until today, and is confusing, because unlike other commands,
`docker push` does not default to use the `:latest` tag when omitted, but instead
makes it push "all tags of the image".

`docker pull` had a similar behavior, but PR 7759 (9c08364a412a51c80e8d17ae14f92549dc543e68)
changed the behavior to default to the `:latest` tag, and added a `--all-tags` flag
to the CLI to optionally pull all images.

This patch implements the API client changes to make `docker push` match the behavior
of `docker pull`, and default to pull a single image, unless the `all` option is passed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 client/image_push.go      | 13 +++---
 client/image_push_test.go | 84 ++++++++++++++++++++++++---------------
 2 files changed, 58 insertions(+), 39 deletions(-)

diff --git a/client/image_push.go b/client/image_push.go
index 49d412ee37..845580d4a4 100644
--- a/client/image_push.go
+++ b/client/image_push.go
@@ -25,15 +25,14 @@ func (cli *Client) ImagePush(ctx context.Context, image string, options types.Im
 		return nil, errors.New("cannot push a digest reference")
 	}
 
-	tag := ""
 	name := reference.FamiliarName(ref)
-
-	if nameTaggedRef, isNamedTagged := ref.(reference.NamedTagged); isNamedTagged {
-		tag = nameTaggedRef.Tag()
-	}
-
 	query := url.Values{}
-	query.Set("tag", tag)
+	if !options.All {
+		ref = reference.TagNameOnly(ref)
+		if tagged, ok := ref.(reference.Tagged); ok {
+			query.Set("tag", tagged.Tag())
+		}
+	}
 
 	resp, err := cli.tryImagePush(ctx, name, query, options.RegistryAuth)
 	if errdefs.IsUnauthorized(err) && options.PrivilegeFunc != nil {
diff --git a/client/image_push_test.go b/client/image_push_test.go
index 59d9e0186b..a690b8f28e 100644
--- a/client/image_push_test.go
+++ b/client/image_push_test.go
@@ -131,50 +131,70 @@ func TestImagePushWithPrivilegedFuncNoError(t *testing.T) {
 func TestImagePushWithoutErrors(t *testing.T) {
 	expectedOutput := "hello world"
 	expectedURLFormat := "/images/%s/push"
-	pullCases := []struct {
+	testCases := []struct {
+		all           bool
 		reference     string
 		expectedImage string
 		expectedTag   string
 	}{
 		{
+			all:           false,
+			reference:     "myimage",
+			expectedImage: "myimage",
+			expectedTag:   "latest",
+		},
+		{
+			all:           false,
+			reference:     "myimage:tag",
+			expectedImage: "myimage",
+			expectedTag:   "tag",
+		},
+		{
+			all:           true,
 			reference:     "myimage",
 			expectedImage: "myimage",
 			expectedTag:   "",
 		},
 		{
-			reference:     "myimage:tag",
+			all:           true,
+			reference:     "myimage:anything",
 			expectedImage: "myimage",
-			expectedTag:   "tag",
+			expectedTag:   "",
 		},
 	}
-	for _, pullCase := range pullCases {
-		client := &Client{
-			client: newMockClient(func(req *http.Request) (*http.Response, error) {
-				expectedURL := fmt.Sprintf(expectedURLFormat, pullCase.expectedImage)
-				if !strings.HasPrefix(req.URL.Path, expectedURL) {
-					return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
-				}
-				query := req.URL.Query()
-				tag := query.Get("tag")
-				if tag != pullCase.expectedTag {
-					return nil, fmt.Errorf("tag not set in URL query properly. Expected '%s', got %s", pullCase.expectedTag, tag)
-				}
-				return &http.Response{
-					StatusCode: http.StatusOK,
-					Body:       ioutil.NopCloser(bytes.NewReader([]byte(expectedOutput))),
-				}, nil
-			}),
-		}
-		resp, err := client.ImagePush(context.Background(), pullCase.reference, types.ImagePushOptions{})
-		if err != nil {
-			t.Fatal(err)
-		}
-		body, err := ioutil.ReadAll(resp)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if string(body) != expectedOutput {
-			t.Fatalf("expected '%s', got %s", expectedOutput, string(body))
-		}
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(fmt.Sprintf("%s,all-tags=%t", tc.reference, tc.all), func(t *testing.T) {
+			client := &Client{
+				client: newMockClient(func(req *http.Request) (*http.Response, error) {
+					expectedURL := fmt.Sprintf(expectedURLFormat, tc.expectedImage)
+					if !strings.HasPrefix(req.URL.Path, expectedURL) {
+						return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
+					}
+					query := req.URL.Query()
+					tag := query.Get("tag")
+					if tag != tc.expectedTag {
+						return nil, fmt.Errorf("tag not set in URL query properly. Expected '%s', got %s", tc.expectedTag, tag)
+					}
+					return &http.Response{
+						StatusCode: http.StatusOK,
+						Body:       ioutil.NopCloser(bytes.NewReader([]byte(expectedOutput))),
+					}, nil
+				}),
+			}
+			resp, err := client.ImagePush(context.Background(), tc.reference, types.ImagePushOptions{
+				All: tc.all,
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			body, err := ioutil.ReadAll(resp)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(body) != expectedOutput {
+				t.Fatalf("expected '%s', got %s", expectedOutput, string(body))
+			}
+		})
 	}
 }