KEMBAR78
Add multi build support by juzuluag · Pull Request #24 · devcontainers/cli · GitHub
Skip to content

Add multi build support #24

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
Merged
merged 15 commits into from
Jun 21, 2022
Merged

Add multi build support #24

merged 15 commits into from
Jun 21, 2022

Conversation

@juzuluag
Copy link
Contributor

@juzuluag juzuluag commented May 19, 2022 •
edited
Loading

Work Item ID

Description

Enhance devcontainer CLI build command by adding an option to build multi-platform container images and to push them to a container registry.

devcontainer build command will be enhanced with the following arguments:
--platform value1, value2. Comma delimited string with platform types [string].
i.e. value: linux/amd64,linux/arm64
--push push to a container registry [boolean] [default: false]

PR Checklist

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran the lint checks which produced no new errors nor warnings for my changes.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.

Does this introduce a breaking change?

  • Yes
  • No

If this introduces a breaking change, please describe the impact and migration path for existing applications below.

Testing

  • What OS was used for testing: Window and MacOS
  • Which test sets were used: mocka and manual testing
  • Description of test scenarios that you have tried.
    manual: Build images and upload them to container registry (docker hub)

Manual testing:

node devcontainer.js build --workspace-folder ../demo_container/  --platform linux/arm64,linux/amd64 --push --image-name dockerhubuname/test1:v1

[+] Building 1.9s (9/9) FINISHED                                                
 => [internal] load build definition from Dockerfile                       0.0s
 => => transferring dockerfile: 245B                                       0.0s
...
 => => pushing manifest for docker.io/dockerhubuname/test1:v1@sha256:00b62d3859  0.4s
 => [auth] dockerhubuname/test1:pull,push token for registry-1.docker.io         0.0s
 => [auth] dockerhubuname/test1:pull,push token for registry-1.docker.io         0.0s

List manifest from container registry

docker buildx imagetools inspect dockerhubuname/test1:v1
Name:      docker.io/dockerhubuname/test1:v1
MediaType: application/vnd.docker.distribution.manifest.list.v2+json
Digest:    sha256:00b62d385953096b5b09ba1458d0e2467b53b3c6d2409958a67eabd68f4fa0a0
           
Manifests: 
  Name:      docker.io/dockerhubuname/test1:v1@sha256:a18591ab9c4d416b9ee5b86c1b66c5ff9b89f8b8b522bc3d26db1395bd736f34
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/arm64
             
  Name:      docker.io/dockerhubuname/test1:v1@sha256:4ae8b07ea9a8c61da4a06548d758fc27709408b79d52d293bdc0edb940041576
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/amd64

Other information or known dependencies

n/a

@juzuluag juzuluag mentioned this pull request May 19, 2022
Closed
8 tasks
@juzuluag juzuluag marked this pull request as ready for review May 19, 2022 21:12
@stuartleeks
Copy link
Collaborator

stuartleeks commented May 20, 2022 •
edited
Loading

(disclaimer: I'm not on the VS Code team, so will defer to @chrmarti - my comments are those of an interested observer and someone who has made some contributions in similar areas of the code 😄)

Hi @juzuluag, this looks pretty cool - being able to create multi-platform dev container images would be really useful I think. Especially building them in CI and publishing so that they can be referenced in the cacheFrom in devcontainer.json (or the equivalent cache_from in docker-compose.yml) and then work across multiple platforms

There has been some recent work here that shifts to using BuildKit/buildx by default (if it is present) for Dockerfile-based dev containers (the work on docker-compose dev containers is still in-flight). Hopefully that will simplify the work to support multi-platform?

With the buildx work, I think that enableBuildx could be dropped in favour of the buildKitVersion checks?
Similarly, the buildxLoad looks like it could be removed as the current buildx path includes the --load argument when calling docker.

For the --push parameter, I'm curious whether there a particular benefit to adding the --push rather than the caller simply executing a docker push command after running the devcontainer build step?

I also think that it would be good to add some tests for the --platform option to make sure that future changes don't break the functionality. Maybe run a build and inspect the image metadata afterwards to check that the expected platforms are included?

stuartleeks
stuartleeks reviewed May 20, 2022
View reviewed changes
@chrmarti chrmarti self-assigned this May 20, 2022
stuartleeks
stuartleeks reviewed May 23, 2022
View reviewed changes
stuartleeks
stuartleeks reviewed May 23, 2022
View reviewed changes
@juzuluag juzuluag requested review from chrmarti and stuartleeks May 23, 2022 20:20
stuartleeks
stuartleeks reviewed May 24, 2022
View reviewed changes
stuartleeks
stuartleeks reviewed May 24, 2022
View reviewed changes
chrmarti
chrmarti reviewed May 24, 2022
View reviewed changes
@juzuluag juzuluag requested review from chrmarti and stuartleeks May 25, 2022 11:15
@juzuluag
Copy link
Contributor Author

Hi @chrmarti and @stuartleeks , any chance to look at the most recent changes? Thanks.

@jMac029
Copy link

jMac029 commented Jun 9, 2022

+1 on this feature request, would be very helpful for my team and our tooling

chrmarti
chrmarti reviewed Jun 10, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Thanks @juzuluag. Added a few more comments.

@chrmarti chrmarti added this to the June 2022 milestone Jun 10, 2022
@juzuluag juzuluag requested a review from chrmarti June 10, 2022 14:21
@kieferrm kieferrm mentioned this pull request Jun 11, 2022
Closed
6 tasks
@samruddhikhandale
Copy link
Member

Could we be able to prioritize this PR? cc @chrmarti

I am working on devcontainers/images to create images based on features by using the devcontainers/cli. We are looking forward to publish these images, but can't yet as they need to be built with --progress flag which this PR targets

chrmarti
chrmarti approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @juzuluag !

@chrmarti chrmarti merged commit 208ebb3 into devcontainers:main Jun 21, 2022
@juzuluag juzuluag deleted the juzuluag/multi_build_merge branch June 22, 2022 01:13
@gregvanl gregvanl mentioned this pull request Jul 19, 2022
Closed
@metaskills metaskills mentioned this pull request Feb 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrmarti chrmarti chrmarti approved these changes

@stuartleeks stuartleeks Awaiting requested review from stuartleeks

Assignees

@chrmarti chrmarti

Labels

None yet

Projects

None yet

Milestone

June 2022

Development

Successfully merging this pull request may close these issues.

5 participants

@juzuluag @stuartleeks @jMac029 @samruddhikhandale @chrmarti