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 multi build support #24

Merged
merged 15 commits into from
Jun 21, 2022

Conversation

juzuluag
Copy link
Contributor

@juzuluag juzuluag commented May 19, 2022

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
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

(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?

@chrmarti chrmarti self-assigned this May 20, 2022
src/spec-node/singleContainer.ts Show resolved Hide resolved
src/spec-node/singleContainer.ts Outdated Show resolved Hide resolved
@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

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.

src/spec-node/singleContainer.ts Outdated Show resolved Hide resolved
src/spec-node/devContainersSpecCLI.ts Show resolved Hide resolved
src/spec-node/devContainersSpecCLI.ts Show resolved Hide resolved
src/spec-node/singleContainer.ts Show resolved Hide resolved
@chrmarti chrmarti added this to the June 2022 milestone Jun 10, 2022
@juzuluag juzuluag requested a review from chrmarti June 10, 2022 14:21
@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

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 !

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.

5 participants