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 support for building OCI multi-arch images #93

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

rdrgmnzs
Copy link
Contributor

@rdrgmnzs rdrgmnzs commented Jun 18, 2022

Add support for building OCI multi-arch images

It should work in conjunction with the work on concourse/registry-image-resource#321

Signed-off-by: Rodrigo Menezes brdude@gmail.com

Copy link
Contributor

@xtremerui xtremerui left a comment

Choose a reason for hiding this comment

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

Thx for the PR. Just some nit questions. And there is tests failure too.

task.go Outdated Show resolved Hide resolved
task.go Outdated Show resolved Hide resolved
task.go Outdated Show resolved Hide resolved
@rdrgmnzs rdrgmnzs force-pushed the multi-arch branch 3 times, most recently from 213f8f6 to 45e59b0 Compare June 26, 2022 04:42
task.go Show resolved Hide resolved
@xtremerui
Copy link
Contributor

Could you add test coverage for the load OCI image method? thx!

@jmccann
Copy link
Contributor

jmccann commented Jun 29, 2022

Do we need to add notes to README.md on how to leverage this? I'm interested to try this out (building from source from this PR) and not sure what I'd need to do in task/pipeline definitions to properly leverage this additional functionality.

Also, thanks @rdrgmnzs for taking this on!!!

@@ -69,6 +69,8 @@ type Config struct {
// Theoretically this would go away if/when we standardize on OCI.
UnpackRootfs bool `json:"unpack_rootfs" envconfig:"optional"`

OutputOCI bool `json:"output_oci" envconfig:"optional"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmccann here you go!

@rdrgmnzs please update readme for this configuration. Thx!

@rdrgmnzs rdrgmnzs force-pushed the multi-arch branch 14 times, most recently from 336cbbf to 5369216 Compare July 13, 2022 04:07
Signed-off-by: Rodrigo Menezes <brdude@gmail.com>
@rdrgmnzs
Copy link
Contributor Author

@xtremerui test added and docs updated!

Comment on lines +612 to +617
desc := im.Manifests[0]
ii, err := l.ImageIndex(desc.Digest)
s.NoError(err)

images, err := ii.IndexManifest()
s.NoError(err)
Copy link
Contributor

@xtremerui xtremerui Jul 13, 2022

Choose a reason for hiding this comment

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

	desc := im.Manifests[0]
	ii, err := l.ImageIndex(desc.Digest)
	s.NoError(err)

	images, err := ii.IndexManifest()
	s.NoError(err)

is this part necessary since im at line 609 is already IndexManifext? Could it use im instead of images at line 621?

Copy link
Contributor Author

@rdrgmnzs rdrgmnzs Jul 13, 2022

Choose a reason for hiding this comment

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

@xtremerui unfortunately it is, I was very confused by this at first as well. The first IndexManifest is for the bundle it self, you then provide the "Digest" of that bundle to get it's index and then you retrieve that manifest using IndexManifest and that's where the data for all the images (multiple images because of multiple CPU architectures) is contained.

When I was figuring this out I actually added print statements to the test, here's a sample output from that so you can see how they differ:

Contents of im: &{SchemaVersion:2 MediaType: Manifests:[{MediaType:application/vnd.oci.image.index.v1+json Size:703 Digest:sha256:1e0d02719eea72a55306e7d55d9d84b6ac25c3ac5705e0d78d72f26eee18e649 Data:[] URLs:[] Annotations:map[org.opencontainers.image.created:2022-07-13T16:48:31Z] Platform:<nil>}] Annotations:map[]} 

Contents of images: &{SchemaVersion:2 MediaType:application/vnd.oci.image.index.v1+json Manifests:[{MediaType:application/vnd.oci.image.manifest.v1+json Size:710 Digest:sha256:a6f9c6d46f97a85ace889664bc42437cff0e59c769b02629d98470378ff659d0 Data:[] URLs:[] Annotations:map[] Platform:linux/arm64} {MediaType:application/vnd.oci.image.manifest.v1+json Size:710 Digest:sha256:84b4c953792a04908939367e04c74baa03d8e4f2e701b1c3aa20751e9d286089 Data:[] URLs:[] Annotations:map[] Platform:linux/amd64}] Annotations:map[]} 

Copy link
Contributor

@xtremerui xtremerui left a comment

Choose a reason for hiding this comment

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

LGTM!

@ghost
Copy link

ghost commented Sep 28, 2022

@xtremerui when is this planned to be released? Would really love to be able to use this feature :)

@xtremerui
Copy link
Contributor

@hugohjerten-spire new release is out!

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.

3 participants