Skip to content

Conversation

@nalind
Copy link
Member

@nalind nalind commented Feb 17, 2020

Similarly to how we fall back to trying to push single-image manifests in alternate formats when the original/preferred type is rejected by a registry, attempt to do the same for manifest lists.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@mtrmac PTAL

@nalind nalind force-pushed the manifest-list-retry-using-conversions branch 3 times, most recently from 00b7353 to 17393e8 Compare February 24, 2020 15:12
@rhatdan
Copy link
Member

rhatdan commented Mar 3, 2020

@mtrmac PTAL

@nalind nalind force-pushed the manifest-list-retry-using-conversions branch 3 times, most recently from c0cd64d to 9821654 Compare March 10, 2020 14:58
@nalind nalind force-pushed the manifest-list-retry-using-conversions branch from 9821654 to 4a936a6 Compare March 12, 2020 20:54
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

// Iterate through supported list types, preferred format first.
c.Printf("Writing manifest list to image destination\n")
var errs []string
for _, thisListType := range append([]string{selectedListType}, otherManifestMIMETypeCandidates...) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Can/should this use ManifestTypeRejectedError to avoid unnecessary attempts (and duplicated error messages) for other kinds of failures?

OTOH right now we only have two types, so it’s not at all urgent.

@vrothberg
Copy link
Member

@nalind, can you address @mtrmac's comments? We want to cut a c/image release this week and this PR would be great to have in.

@nalind nalind force-pushed the manifest-list-retry-using-conversions branch from 4a936a6 to 5e404aa Compare March 18, 2020 15:08
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@vrothberg
Copy link
Member

Tests aren't happy:

FAIL: copy_test.go:134: CopySuite.TestCopyAllWithManifestListConverge

Running skopeo copy --all docker://estesp/busybox:latest oci:/tmp/copy-all-manifest-list-oci197628618

Running skopeo copy --all oci:/tmp/copy-all-manifest-list-oci197628618 dir:/tmp/copy-all-manifest-list-dir954760332

Running skopeo copy --all --format oci docker://estesp/busybox:latest dir:/tmp/copy-all-manifest-list-dir325661819

Running skopeo copy --all dir:/tmp/copy-all-manifest-list-dir325661819 oci:/tmp/copy-all-manifest-list-oci697180577

copy_test.go:151:

    assertDirImagesAreEqual(c, dir1, dir2)

copy_test.go:681:

    c.Assert(digests[0], check.Equals, digests[1])

... obtained digest.Digest = "sha256:ba544e9c859530821721b98ff2f61e9a224847f201d3080dfb9ced3e590f2e78"

... expected digest.Digest = "sha256:8f7df246f4fd3e26765a837cea85fc2f7a7f793412eda44cca53fbddce9957f6"

... Difference:

...     "sha256:ba544e9c859530821721b98ff2f61e9a224847f201d3080dfb9ced3e590f2e78" != "sha256:8f7df246f4fd3e26765a837cea85fc2f7a7f793412eda44cca53fbddce9957f6"

Similarly to how we fall back to trying to push single-image manifests
in alternate formats when the original/preferred type is rejected by a
registry, attempt to do the same for manifest lists.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind nalind force-pushed the manifest-list-retry-using-conversions branch from 5e404aa to 062db46 Compare March 18, 2020 17:42
@rhatdan
Copy link
Member

rhatdan commented Mar 18, 2020

LGTM

@rhatdan rhatdan merged commit 2e70342 into containers:master Mar 18, 2020
@nalind nalind deleted the manifest-list-retry-using-conversions branch March 18, 2020 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants