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

Update pulumi dockerfile to multiarch #274

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

MMartyn
Copy link
Contributor

@MMartyn MMartyn commented Sep 12, 2024

I would like to setup devcontainers for use by the developers when they are working on my custom provider. Currently since there isn't an arm64 version, the experience is slow and painful on Apple Silicon. This change updates the Dockerfile so that all that is needed is the --platform=linux/arm64 in the build and it will take care of switching out all the places needed to build. Note: The build matrices will need to be updated if this is something you would consider adding.

Thanks.

@MMartyn MMartyn requested a review from a team as a code owner September 12, 2024 22:21
Copy link
Contributor

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

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

Thanks! I do think this would be really useful to have. I left a few comments inline. Would you be willing to update the build matrix as well?

@julienp is the expert on these docker files, so I would want him to have a look at this as well (he's on PTO, and will be back next week)

docker/pulumi/Dockerfile Outdated Show resolved Hide resolved
docker/pulumi/Dockerfile Outdated Show resolved Hide resolved
docker/pulumi/Dockerfile Show resolved Hide resolved
MMartyn and others added 3 commits September 25, 2024 10:21
- Switched the conditionals to default to amd64
- Added a link to docs on built in build args as well as inform the potential values
- Added build matrix for creating the multiarch images
@MMartyn
Copy link
Contributor Author

MMartyn commented Sep 25, 2024

I believe I addressed all of the comments. Looking at the addition of nonroot, I think all is well there as well.

@julienp julienp self-requested a review September 25, 2024 14:41
@julienp julienp self-assigned this Sep 25, 2024
@julienp
Copy link
Contributor

julienp commented Sep 25, 2024

This looks great! I think we also need to matrix the arch in ci.yml. That's only running the tests, so shouldn't need all the work for the docker manifests.

Annoyingly, there's also sync-ecr.yml and sync-ghcr.yml that push the images to the GitHub and amazon registries, which probably need to recreate the manifests again.

@julienp julienp mentioned this pull request Sep 25, 2024
@MMartyn
Copy link
Contributor Author

MMartyn commented Sep 25, 2024

BTW regarding the build failure, seems like a transient GH issue https://github.com/orgs/community/discussions/139743

@MMartyn
Copy link
Contributor Author

MMartyn commented Sep 25, 2024

Added the code for gcp and aws. Didn't touch the ci one since it looks like all the other ones just use the amd64 version for testing.

@julienp
Copy link
Contributor

julienp commented Sep 26, 2024

Added the code for gcp and aws. Didn't touch the ci one since it looks like all the other ones just use the amd64 version for testing.

I think we do test the language specific images on arm64, but I'll create a separate issue for myself to look into this for the kitchen sink image. There will likely be some issues due to us using qemu to test arm64, I know at lest dotnet can't run under qemu, and there might be some issues with nodejs also.

@julienp
Copy link
Contributor

julienp commented Sep 26, 2024

Test failure on this PR are because we aren't setup well for running tests for external contributions. I ran the code changes on this branch here https://github.com/pulumi/pulumi-docker-containers/actions/runs/11035725145/job/30690370889?pr=285

Copy link
Contributor

@julienp julienp left a comment

Choose a reason for hiding this comment

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

Resolved a merge conflict due to 2a13fe5, which in hindsight I could have merged this PR instead and it would have fixed the issue also, since it splits the push for the latest tag out to the manifests step!

@julienp julienp merged commit 47fdf65 into pulumi:main Sep 26, 2024
3 of 39 checks passed
@julienp
Copy link
Contributor

julienp commented Sep 27, 2024

Unfortunately we had to disable the release builds of these images on arm64, because it takes too long to install the Python versions provided by the image when running in Docker + QEMU.

Tracking re-enabling these here #297

In the meantime, the images can be built locally on an arm64 machine.

@MMartyn
Copy link
Contributor Author

MMartyn commented Sep 27, 2024

Alright, looks like they plan on opening it up to open source by EOY. I went ahead and built the images and pushed them to my ECR so I can get my internal devs the ability to use devcontainers. I'll keep my eye on that #297

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