Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manifest List Interface #191

Merged
merged 189 commits into from
May 7, 2024
Merged

Conversation

husni-faiz
Copy link
Contributor

@husni-faiz husni-faiz commented Apr 5, 2023

Hi all,

I am working on the issue #188 to create a ManifestList/ImageIndex interface as part of a larger scope of adding multi-architecture support to buildpacks. I would appreciate your ideas and thoughts on how we want to move forward. There is a lot of validations and test to be added along the line.

In parallel a pack-cli command manifest is being developed to work with Manifest Lists.

You may find the following resources useful.

Fixes #188

Copy link
Member

@jjbustamante jjbustamante left a comment

Choose a reason for hiding this comment

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

Keep working on this

local/index.go Outdated Show resolved Hide resolved
local/index.go Outdated Show resolved Hide resolved
@husni-faiz husni-faiz force-pushed the dev-image-index branch 5 times, most recently from f7314e6 to 0ca19aa Compare June 5, 2023 14:47
@husni-faiz husni-faiz force-pushed the dev-image-index branch 5 times, most recently from 742529c to 1c5039d Compare November 20, 2023 16:39
The OCI spec defines an Image Index concept to handle
multiple manifests in an OCI image. This adds the interface
for Image Index.

Signed-off-by: Husni Faiz <[email protected]>
remote.Image uses descriptor to get the image. Getting the descriptor
first and deriving the image from the descriptor is efficient instead
of getting the descriptor twice.

Signed-off-by: Husni Faiz <[email protected]>
options.go Outdated Show resolved Hide resolved
remote/new.go Outdated

// ValidateRepoName
// TODO move this code to something more generic
func validateRepoName(repoName string, o *imgutil.IndexOptions) error {
Copy link
Member

Choose a reason for hiding this comment

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

This function will be more re-usable if it just accepts an insecure bool, instead of index options. Then we can use it for images too.

Copy link
Member

Choose a reason for hiding this comment

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

I agreed, honestly I put it here just to remember me to do something with it later!

Copy link
Member

Choose a reason for hiding this comment

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

Just a note, we're not actually validating the "repo name" for remote images today (though maybe we should). I think I removed it in husni-faiz#12, but we could add it back.

layout/new.go Outdated Show resolved Hide resolved
layout/new.go Outdated Show resolved Hide resolved
new.go Outdated Show resolved Hide resolved
layout/new.go Outdated Show resolved Hide resolved
remote/new.go Outdated Show resolved Hide resolved
jjbustamante and others added 9 commits April 19, 2024 13:38
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
…oid getting rate limited

Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
… save the image information. Reimplement the getters/setters to read the information from the index. Adding more test coverage

Signed-off-by: Juan Bustamante <[email protected]>
if err != nil {
return "", err
}
return configFile.OS, nil
if desc.Platform != nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: getDescriptorFrom never returns nil in its first argument, so I think you can remove this check

Copy link
Member

Choose a reason for hiding this comment

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

I think I added it because on unit testing when using random packages in ggcr some nil in these attributes but let me check if everything is file after removing it

cnb_index.go Outdated
multiWriteTagables[ref.Context().Tag(tag)] = taggableIndex
}

// FIXME: this will only push the index manifest, assuming that all the images it refers to exist in the registry
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not a FIXME, I tried to remove it and use the remote.WriteIndex but in such a case we need to have everything on disk in OCI layout format.
I think we should document this behavior for library consumers. For example:

  • I want to create my own index at repo/foo and include some busybox manifests, first you need to copy the busybox manifests to repo/foo before pushing the index otherwise it fails

cnb_index.go Outdated Show resolved Hide resolved
@natalieparellano natalieparellano requested a review from jabrown85 May 1, 2024 15:23
@natalieparellano
Copy link
Member

@jabrown85 I think this one is just about ready if you'd like to take a look. I turned on squashing for PRs so that we can bring this into main as one commit 😅

Signed-off-by: Juan Bustamante <[email protected]>
@natalieparellano natalieparellano merged commit 9f7b96c into buildpacks:main May 7, 2024
3 of 5 checks passed
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.

Create an new interface to handle Manifest Lists
6 participants