Repository: google/go-containerregistry
Pull request #818 (2020-11-06 20:05:40)

Make sure options are propagated to child Writes
Right now we are just passing through cherrypicked options from
WriteIndex to other WriteIndex and Write calls. This isn't sustainable
as we add more options, since we'd have to keep these in sync.

Instead, for WriteIndex, we can reuse the writer by pulling the
implementation out into writer.writeIndex.

For Write, since we do scope calculation based on layers, we can't reuse
the writer because we don't do scope calculation for WriteIndex.

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/818?src=pr&el=h1) Report
> Merging [#818](https://codecov.io/gh/google/go-containerregistry/pull/818?src=pr&el=desc) (91e7f5a) into [master](https://codecov.io/gh/google/go-containerregistry/commit/6a6302569e968bb450a96bc80af791aa689d53a3?el=desc) (6a63025) will **decrease** coverage by `0.06%`.
> The diff coverage is `53.57%`.

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

```diff
@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
- Coverage   74.77%   74.71%   -0.07%     
==========================================
  Files         103      103              
  Lines        4313     4314       +1     
==========================================
- Hits         3225     3223       -2     
- Misses        611      614       +3     
  Partials      477      477              
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/818?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/multi\_write.go](https://codecov.io/gh/google/go-containerregistry/pull/818/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9tdWx0aV93cml0ZS5nbw==) | `57.54% <0.00%> (ø)` | |
| [pkg/v1/remote/write.go](https://codecov.io/gh/google/go-containerregistry/pull/818/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS93cml0ZS5nbw==) | `65.09% <55.55%> (+0.12%)` | :arrow_up: |
| [pkg/v1/remote/options.go](https://codecov.io/gh/google/go-containerregistry/pull/818/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9vcHRpb25zLmdv) | `61.53% <0.00%> (-7.70%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/818?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/818?src=pr&el=footer). Last update [6a63025...e237685](https://codecov.io/gh/google/go-containerregistry/pull/818?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/v1/remote/multi_write.go b/pkg/v1/remote/multi_write.go
index 259348d2..631ab23f 100644
--- a/pkg/v1/remote/multi_write.go
+++ b/pkg/v1/remote/multi_write.go
@@ -127,7 +127,7 @@ func MultiWrite(m map[name.Reference]Taggable, options ...Option) error {
 			// Start N workers consuming tasks to upload manifests.
 			g.Go(func() error {
 				for t := range taskChan {
-					if err := w.commitImage(t.i, t.ref); err != nil {
+					if err := w.commitManifest(t.i, t.ref); err != nil {
 						return err
 					}
 				}
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index 3115e550..dec92bd2 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -128,7 +128,7 @@ func Write(ref name.Reference, img v1.Image, options ...Option) error {
 
 	// With all of the constituent elements uploaded, upload the manifest
 	// to commit the image.
-	return w.commitImage(img, ref)
+	return w.commitManifest(img, ref)
 }
 
 // writer writes the elements of an image to a remote image reference.
@@ -376,6 +376,53 @@ func (w *writer) uploadOne(l v1.Layer) error {
 	return retry.Retry(tryUpload, retry.IsTemporary, backoff)
 }
 
+func (w *writer) writeIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error {
+	index, err := ii.IndexManifest()
+	if err != nil {
+		return err
+	}
+
+	// TODO(#803): Pipe through remote.WithJobs and upload these in parallel.
+	for _, desc := range index.Manifests {
+		ref := ref.Context().Digest(desc.Digest.String())
+		exists, err := w.checkExistingManifest(desc.Digest, desc.MediaType)
+		if err != nil {
+			return err
+		}
+		if exists {
+			logs.Progress.Print("existing manifest: ", desc.Digest)
+			continue
+		}
+
+		switch desc.MediaType {
+		case types.OCIImageIndex, types.DockerManifestList:
+			ii, err := ii.ImageIndex(desc.Digest)
+			if err != nil {
+				return err
+			}
+
+			if err := w.writeIndex(ref, ii); err != nil {
+				return err
+			}
+		case types.OCIManifestSchema1, types.DockerManifestSchema2:
+			img, err := ii.Image(desc.Digest)
+			if err != nil {
+				return err
+			}
+			// TODO: Ideally we could reuse this writer, but we need to know
+			// scopes before we do the token exchange. To be lazy here, just
+			// re-do the token exchange. MultiWrite fixes this.
+			if err := Write(ref, img, options...); err != nil {
+				return err
+			}
+		}
+	}
+
+	// With all of the constituent elements uploaded, upload the manifest
+	// to commit the image.
+	return w.commitManifest(ii, ref)
+}
+
 type withMediaType interface {
 	MediaType() (types.MediaType, error)
 }
@@ -418,8 +465,8 @@ func unpackTaggable(t Taggable) (*v1.Descriptor, error) {
 	}, nil
 }
 
-// commitImage does a PUT of the image's manifest.
-func (w *writer) commitImage(t Taggable, ref name.Reference) 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
@@ -482,11 +529,6 @@ func scopesForUploadingImage(repo name.Repository, layers []v1.Layer) []string {
 // WriteIndex will attempt to push all of the referenced manifests before
 // attempting to push the ImageIndex, to retain referential integrity.
 func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error {
-	index, err := ii.IndexManifest()
-	if err != nil {
-		return err
-	}
-
 	o, err := makeOptions(ref.Context(), options...)
 	if err != nil {
 		return err
@@ -501,43 +543,7 @@ func WriteIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error {
 		client:  &http.Client{Transport: tr},
 		context: o.context,
 	}
-
-	// TODO(#803): Pipe through remote.WithJobs and upload these in parallel.
-	for _, desc := range index.Manifests {
-		ref := ref.Context().Digest(desc.Digest.String())
-		exists, err := w.checkExistingManifest(desc.Digest, desc.MediaType)
-		if err != nil {
-			return err
-		}
-		if exists {
-			logs.Progress.Printf("existing manifest: %v", desc.Digest)
-			continue
-		}
-
-		switch desc.MediaType {
-		case types.OCIImageIndex, types.DockerManifestList:
-			ii, err := ii.ImageIndex(desc.Digest)
-			if err != nil {
-				return err
-			}
-
-			if err := WriteIndex(ref, ii, WithAuth(o.auth), WithTransport(o.transport)); err != nil {
-				return err
-			}
-		case types.OCIManifestSchema1, types.DockerManifestSchema2:
-			img, err := ii.Image(desc.Digest)
-			if err != nil {
-				return err
-			}
-			if err := Write(ref, img, WithAuth(o.auth), WithTransport(o.transport)); err != nil {
-				return err
-			}
-		}
-	}
-
-	// With all of the constituent elements uploaded, upload the manifest
-	// to commit the image.
-	return w.commitImage(ii, ref)
+	return w.writeIndex(ref, ii, options...)
 }
 
 // WriteLayer uploads the provided Layer to the specified repo.
@@ -573,7 +579,7 @@ func Tag(tag name.Tag, t Taggable, options ...Option) error {
 	// * Tag could take a list of tags.
 	// * Allow callers to pass in a transport.Transport, typecheck
 	//   it to allow them to reuse the transport across multiple calls.
-	// * WithTag option to do multiple manifest PUTs in commitImage.
+	// * WithTag option to do multiple manifest PUTs in commitManifest.
 	tr, err := transport.New(tag.Context().Registry, o.auth, o.transport, scopes)
 	if err != nil {
 		return err
@@ -584,5 +590,5 @@ func Tag(tag name.Tag, t Taggable, options ...Option) error {
 		context: o.context,
 	}
 
-	return w.commitImage(t, tag)
+	return w.commitManifest(t, tag)
 }


Test patch:
diff --git a/pkg/v1/remote/write_test.go b/pkg/v1/remote/write_test.go
index 12f0f8a9..92ce1802 100644
--- a/pkg/v1/remote/write_test.go
+++ b/pkg/v1/remote/write_test.go
@@ -813,8 +813,8 @@ func TestCommitImage(t *testing.T) {
 	}
 	defer closer.Close()
 
-	if err := w.commitImage(img, w.repo.Tag("latest")); err != nil {
-		t.Errorf("commitImage() = %v", err)
+	if err := w.commitManifest(img, w.repo.Tag("latest")); err != nil {
+		t.Error("commitManifest() = ", err)
 	}
 }