Repository: google/go-containerregistry
Pull request #863 (2020-12-08 16:51:45)

Expand godoc for remote.Tag
Fixes https://github.com/google/go-containerregistry/issues/862

Also, refactor unpackTaggable a bit to ensure we only call RawManifest once.

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=h1) Report
> Merging [#863](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=desc) (e6f96a3) into [master](https://codecov.io/gh/google/go-containerregistry/commit/f39a19656a034cc3eb22e8ca6e4eda08ed59a8c6?el=desc) (f39a196) will **increase** coverage by `0.00%`.
> The diff coverage is `57.14%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/863/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4)](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #863   +/-   ##
=======================================
  Coverage   74.29%   74.29%           
=======================================
  Files         106      106           
  Lines        4454     4451    -3     
=======================================
- Hits         3309     3307    -2     
  Misses        651      651           
+ Partials      494      493    -1     
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/write.go](https://codecov.io/gh/google/go-containerregistry/pull/863/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS93cml0ZS5nbw==) | `65.59% <57.14%> (+0.34%)` | :arrow_up: |
| [pkg/v1/remote/descriptor.go](https://codecov.io/gh/google/go-containerregistry/pull/863/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9kZXNjcmlwdG9yLmdv) | `73.78% <0.00%> (-0.61%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=footer). Last update [f39a196...e6f96a3](https://codecov.io/gh/google/go-containerregistry/pull/863?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/v1/remote/transport/ping.go b/pkg/v1/remote/transport/ping.go
index 6ed1d464..396d4e03 100644
--- a/pkg/v1/remote/transport/ping.go
+++ b/pkg/v1/remote/transport/ping.go
@@ -16,6 +16,7 @@ package transport
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -78,7 +79,7 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 		schemes = append(schemes, "http")
 	}
 
-	var connErr error
+	var errs []string
 	for _, scheme := range schemes {
 		url := fmt.Sprintf("%s://%s/v2/", scheme, reg.Name())
 		req, err := http.NewRequest(http.MethodGet, url, nil)
@@ -87,7 +88,7 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 		}
 		resp, err := client.Do(req.WithContext(ctx))
 		if err != nil {
-			connErr = err
+			errs = append(errs, err.Error())
 			// Potentially retry with http.
 			continue
 		}
@@ -124,5 +125,5 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 			return nil, CheckError(resp, http.StatusOK, http.StatusUnauthorized)
 		}
 	}
-	return nil, connErr
+	return nil, errors.New(strings.Join(errs, "; "))
 }
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index 521b2250..d850203d 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -448,13 +448,13 @@ type withMediaType interface {
 //
 // Use reflection to either pull the v1.Descriptor out of remote.Descriptor or
 // create a descriptor based on the RawManifest and (optionally) MediaType.
-func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
+func unpackTaggable(t Taggable) ([]byte, *v1.Descriptor, error) {
 	if d, ok := t.(*Descriptor); ok {
-		return &d.Descriptor, nil
+		return d.Manifest, &d.Descriptor, nil
 	}
 	b, err := t.RawManifest()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// A reasonable default if Taggable doesn't implement MediaType.
@@ -463,17 +463,17 @@ func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
 	if wmt, ok := t.(withMediaType); ok {
 		m, err := wmt.MediaType()
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		mt = m
 	}
 
 	h, sz, err := v1.SHA256(bytes.NewReader(b))
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return &v1.Descriptor{
+	return b, &v1.Descriptor{
 		MediaType: mt,
 		Size:      sz,
 		Digest:    h,
@@ -482,11 +482,7 @@ func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
 
 // commitManifest does a PUT of the image's manifest.
 func (w *writer) commitManifest(t Taggable, ref name.Reference) error {
-	raw, err := t.RawManifest()
-	if err != nil {
-		return err
-	}
-	desc, err := unpackTaggable(t)
+	raw, desc, err := unpackTaggable(t)
 	if err != nil {
 		return err
 	}
@@ -511,7 +507,7 @@ func (w *writer) commitManifest(t Taggable, ref name.Reference) error {
 	}
 
 	// The image was successfully pushed!
-	logs.Progress.Printf("%v: digest: %v size: %d", ref, desc.Digest, len(raw))
+	logs.Progress.Printf("%v: digest: %v size: %d", ref, desc.Digest, desc.Size)
 	return nil
 }
 
@@ -581,7 +577,17 @@ func WriteLayer(repo name.Repository, layer v1.Layer, options ...Option) error {
 	return w.uploadOne(layer)
 }
 
-// Tag adds a tag to the given Taggable.
+// Tag adds a tag to the given Taggable via PUT /v2/.../manifests/<tag>
+//
+// Notable implementations of Taggable are v1.Image, v1.ImageIndex, and
+// remote.Descriptor.
+//
+// If t implements MediaType, we will use that for the Content-Type, otherwise
+// we will default to types.DockerManifestSchema2.
+//
+// Tag does not attempt to write anything other than the manifest, so callers
+// should ensure that all blobs or manifests that are referenced by t exist
+// in the target registry.
 func Tag(tag name.Tag, t Taggable, options ...Option) error {
 	o, err := makeOptions(tag.Context(), options...)
 	if err != nil {


Test patch:
diff --git a/pkg/v1/remote/transport/ping_test.go b/pkg/v1/remote/transport/ping_test.go
index 6131cf28..aa3a42f3 100644
--- a/pkg/v1/remote/transport/ping_test.go
+++ b/pkg/v1/remote/transport/ping_test.go
@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -169,22 +170,26 @@ func TestPingHttpFallback(t *testing.T) {
 	tests := []struct {
 		reg       name.Registry
 		wantCount int
+		err       string
+		contains  []string
 	}{{
 		reg:       mustRegistry("gcr.io"),
 		wantCount: 1,
+		err:       `Get "https://gcr.io/v2/": http: server gave HTTP response to HTTPS client`,
 	}, {
 		reg:       mustRegistry("ko.local"),
 		wantCount: 2,
 	}, {
 		reg:       mustInsecureRegistry("us.gcr.io"),
-		wantCount: 2,
+		wantCount: 0,
+		contains:  []string{"https://us.gcr.io/v2/", "http://us.gcr.io/v2/"},
 	}}
 
 	gotCount := 0
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			gotCount++
-			http.Error(w, "Forbidden", http.StatusForbidden)
+			w.WriteHeader(http.StatusOK)
 		}))
 	defer server.Close()
 
@@ -195,15 +200,32 @@ func TestPingHttpFallback(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		pr, err := ping(context.Background(), test.reg, tprt)
-		if err == nil {
-			t.Errorf("ping() = %v", pr)
+		// This is the last one, fatal error it.
+		if strings.Contains(test.reg.String(), "us.gcr.io") {
+			server.Close()
 		}
 
-		if test.wantCount != gotCount {
-			t.Errorf("%s: got %d requests, wanted %d", test.reg.String(), test.wantCount, gotCount)
+		_, err := ping(context.Background(), test.reg, tprt)
+		if got, want := gotCount, test.wantCount; got != want {
+			t.Errorf("%s: got %d requests, wanted %d", test.reg.String(), got, want)
 		}
 		gotCount = 0
+
+		if err == nil {
+			if test.err != "" {
+				t.Error("expected err, got nil")
+			}
+			continue
+		}
+		if len(test.contains) != 0 {
+			for _, c := range test.contains {
+				if !strings.Contains(err.Error(), c) {
+					t.Errorf("expected err to contain %q but did not: %q", c, err)
+				}
+			}
+		} else if got, want := err.Error(), test.err; got != want {
+			t.Errorf("got %q want %q", got, want)
+		}
 	}
 }