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.

Sign up for GitHub

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

Jump to bottom

Add builder OCI layout build context #3327

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Add builder OCI layout build context #3327

merged 1 commit into from
Aug 22, 2024

Conversation

austinvazquez
Copy link
Contributor

@austinvazquez austinvazquez commented Aug 18, 2024 •
edited
Loading

Description

This change adds building images with OCI layout build contexts.

Testing

  • Added unit tests
    • Negative test cases
  • Added integration test (Linux only)
    • Test requires build functionality which excludes Windows platform testing.

Additional references

@austinvazquez austinvazquez marked this pull request as ready for review August 18, 2024 17:53
AkihiroSuda
AkihiroSuda reviewed Aug 19, 2024
View reviewed changes
cmd/nerdctl/builder_build_linux_test.go Outdated
)

func TestBuildContextWithOCILayout(t *testing.T) {
testutil.DockerIncompatible(t)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment to explain this

@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch from e29ea5c to 36beb7c Compare August 19, 2024 05:34
AkihiroSuda
AkihiroSuda reviewed Aug 19, 2024
View reviewed changes
cmd/nerdctl/builder_build_linux_test.go Outdated
)

func TestBuildContextWithOCILayout(t *testing.T) {
// Docker driver does not support OCI exporter.
Copy link
Member

Choose a reason for hiding this comment

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

docker buildx build supports it though

Copy link
Contributor Author

@austinvazquez austinvazquez Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @AkihiroSuda, that helped me figure out my issue. I needed to setup a docker-container builder in order to be able to build the OCI tarball.

@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch 2 times, most recently from ade08c6 to f2e337a Compare August 19, 2024 15:46
@austinvazquez
Copy link
Contributor Author

Looks like my test needs a little more bake time. Looking into it now.

@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch from f2e337a to a11a63c Compare August 19, 2024 16:37
@austinvazquez
Copy link
Contributor Author

Got it. Just forgot to tag the image before running. Test now passes on nerdctl and Docker.

@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch from a11a63c to 669e810 Compare August 19, 2024 17:06
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Aug 20, 2024
apostasie
apostasie reviewed Aug 20, 2024
View reviewed changes
pkg/cmd/builder/build.go Outdated Show resolved Hide resolved
apostasie
apostasie reviewed Aug 20, 2024
View reviewed changes
pkg/cmd/builder/build.go Outdated Show resolved Hide resolved
@apostasie
Copy link
Contributor

@austinvazquez left a few comments / nits, but otherwise looks fine.

@austinvazquez
Copy link
Contributor Author

Thanks @apostasie, taking a look now. :)

@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch 5 times, most recently from bedfd3f to 03fbad3 Compare August 21, 2024 02:46
AkihiroSuda
AkihiroSuda reviewed Aug 21, 2024
View reviewed changes
go.mod Outdated
@@ -34,6 +34,7 @@ require (
github.com/coreos/go-iptables v0.7.0
github.com/coreos/go-systemd/v22 v22.5.0
github.com/cyphar/filepath-securejoin v0.3.1
github.com/distribution/distribution v2.8.3+incompatible
Copy link
Member

Choose a reason for hiding this comment

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

This dependency should not be needed

AkihiroSuda
AkihiroSuda reviewed Aug 21, 2024
View reviewed changes
pkg/cmd/builder/build.go Outdated

var digest string
for _, manifest := range ociIndex.Manifests {
if manifest.MediaType == schema2.MediaTypeManifest || manifest.MediaType == ocispec.MediaTypeImageManifest {
Copy link
Member

Choose a reason for hiding this comment

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

You can use https://github.com/containerd/containerd/blob/v2.0.0-rc.3/core/images/mediatypes.go#L40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙌🏼

Copy link
Contributor Author

@austinvazquez austinvazquez Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @AkihiroSuda, pushed the change to use containerd images pkg. Even found a helper function to call in place of the logic I pushed before.

@austinvazquez
Add builder OCI layout build context
1201665 
Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez austinvazquez force-pushed the feat-add-builder-build-oci-layout-build-context branch from 03fbad3 to 1201665 Compare August 21, 2024 03:34
AkihiroSuda
AkihiroSuda approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit de16ae6 into containerd:main Aug 22, 2024
21 checks passed
@austinvazquez austinvazquez deleted the feat-add-builder-build-oci-layout-build-context branch August 22, 2024 02:39
@github-actions github-actions bot mentioned this pull request Nov 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apostasie apostasie apostasie left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@pendo324 pendo324 pendo324 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@austinvazquez @apostasie @pendo324 @AkihiroSuda