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

Add a new manifest command to support multi-architecture builds #1705

Merged
merged 97 commits into from
May 8, 2024

Conversation

husni-faiz
Copy link
Contributor

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

Hi all,

Multi-architecture support for buildpacks have been requested (#1459 | #1460 ) for a long time. Recently I have been working on this as part of the Linux Foundation Mentorship program under the mentorship of Juan Bustamante (@jjbustamante), Jerico Pena (@jericop) and Aidan Delaney (@AidanDelaney)

We 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. We are having a weekly meetings for multi-arch support every Monday @ 10:00 AM EDT/EST. You can find the zoom link in the meeting minutes document.

A new ImageIndex/ManifestList interface is being added to imgutil module.

You may find the following resources useful.

Fixes #1678
Fixes #1460
Fixes #1722
Fixes #1720
Fixes #1721
Fixes #1719
Fixes #1718

@github-actions github-actions bot added this to the 0.30.0 milestone Apr 5, 2023
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement. labels Apr 5, 2023
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.

It is looking good, let's work on the unit testing and error handling

internal/commands/manifest_add.go Outdated Show resolved Hide resolved
internal/commands/manifest_annotate.go Outdated Show resolved Hide resolved
internal/commands/manifest_create.go Outdated Show resolved Hide resolved
pkg/client/create_manifest.go Outdated Show resolved Hide resolved
@natalieparellano natalieparellano modified the milestones: 0.30.0, 0.31.0 May 19, 2023
@github-actions github-actions bot modified the milestones: 0.31.0, 0.30.0 May 23, 2023
@natalieparellano natalieparellano modified the milestones: 0.30.0, 0.31.0 May 30, 2023
@github-actions github-actions bot modified the milestones: 0.31.0, 0.30.0 Jun 1, 2023
Add a new manifest command to pack root commands and add a create
subcommand to the manifest command.

Signed-off-by: Husni Faiz <[email protected]>
Use source/sinks instead of using primitives directly

Signed-off-by: Husni Faiz <[email protected]>
Signed-off-by: Husni Faiz <[email protected]>
Function templates for the following manifest sub command logics
- create
- add
- annotate
- push
- remove

Signed-off-by: Husni Faiz <[email protected]>
Command templates for the following manifest sub commands
- Add
- Annotate
- Push
- Remove

Signed-off-by: Husni Faiz <[email protected]>
@natalieparellano
Copy link
Member

natalieparellano commented May 1, 2024

Great work @husni-faiz @jjbustamante :) I left a few code suggestions and a few comments, but the only ones that are truly blocking for me are

Edit: on the last comment, I think that is actually what we are doing. I'll leave a code suggestion...

Signed-off-by: Juan Bustamante <[email protected]>
@jjbustamante
Copy link
Member

@natalieparellano Thanks a lot for the feedback

I just refactored the files

I changed it for oci or docker default value is oci

Yeah, I think what is missing here is to add some validation to accept --insecure when --publish flag is used.

pkg/index/index_factory_test.go Outdated Show resolved Hide resolved
pkg/index/index_factory_test.go Outdated Show resolved Hide resolved
it("finds the index in the remote registry", func() {
imageIndex, err = indexFactory.FindIndex(indexRepoName)
h.AssertNil(t, err)
h.AssertNotNil(t, imageIndex)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to assert anything about the index, such as its name?

Copy link
Member

Choose a reason for hiding this comment

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

We could, but we don't have any method to expose those attributes.

natalieparellano pushed a commit to buildpacks/imgutil that referenced this pull request May 7, 2024
Implement image index interface in support of https://github.com/buildpacks/rfcs/blob/main/text/0124-pack-manifest-list-commands.md
and buildpacks/pack#1705

Signed-off-by: Husni Faiz <[email protected]>
Signed-off-by: Juan Bustamante <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Sai Kiran <[email protected]>
Signed-off-by: Sai Kiran Maggidi <[email protected]>
Signed-off-by: WYGIN <[email protected]>
Signed-off-by: WYGIN <[email protected]>
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

LGTM (blocked on #2136)

@jjbustamante jjbustamante merged commit 67feb16 into buildpacks:main May 8, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment