Skip to content

Add multi build support#24

Merged
chrmarti merged 15 commits intodevcontainers:mainfrom
juzuluag:juzuluag/multi_build_merge
Jun 21, 2022
Merged

Add multi build support#24
chrmarti merged 15 commits intodevcontainers:mainfrom
juzuluag:juzuluag/multi_build_merge

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

juzuluag commented May 31, 2022

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

samruddhikhandale commented Jun 15, 2022

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