Skip to content

c8d/exporter: Use WithSkipMissing#46978

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:c8d-fix-export
Jan 16, 2024
Merged

c8d/exporter: Use WithSkipMissing#46978
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:c8d-fix-export

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Dec 21, 2023

Save the unmodified manifest list to keep the image ID of the multi-platform images when not all platforms are present.

Note: Without moby/buildkit#4558 vendored copying the distribution source labels into the index.json annotations won't work: containerd/containerd@b9af453 (for OCI archives exported directly from buildkit).

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/images Image Distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Dec 21, 2023
@vvoland vvoland added this to the v-future milestone Dec 21, 2023
@vvoland vvoland self-assigned this Dec 21, 2023
return presentManifests[0], nil
} else if len(presentManifests) == 0 {
// Return error when none of the image's manifest is present.
return none, errdefs.NotFound(fmt.Errorf("none of the manifests is fully present in the content store"))
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we want to have a utility / check around to allow returning a warning.

I think it makes sense to allow exporting "whatever is in the store" (even if partial content), but possibly we want to have;

  • a warning
  • an option to create a new variant of the image with only the available variants (and thus; new digest)

Not exactly sure yet what the "UX" for it would be, and if that would require adding an option to the API ("export policy" or whatever)

@thaJeztah
Copy link
Member

@thaJeztah
Copy link
Member

containerd 1.7.12 was tagged, and I updated my existing PR that updates it (and moves some dependency);

that should unblock this PR

Save the unmodified manifest list to keep the image ID of the
multi-platform images when not all platforms are present.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@thaJeztah thaJeztah marked this pull request as ready for review January 16, 2024 18:29
@thaJeztah
Copy link
Member

Moved this out of draft; IIUC, the remaining bits in BuildKit are "nice to have" but those missing are not troublesome; this implementation is a lot cleaner though to what was there before, so I think it'd be nice to have this in.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

but perhaps @rumpl and/or @dmcgowan can give a second review

@thaJeztah thaJeztah modified the milestones: v-future, 25.0.0 Jan 16, 2024
@thaJeztah
Copy link
Member

Let me bring this one in 👍

@thaJeztah thaJeztah merged commit 2c47a6d into moby:master Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/images Image Distribution containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

4 participants