-
Notifications
You must be signed in to change notification settings - Fork 187
chore(infra): remove --platform build flags from Dockerfiles #2242
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
base: main
Are you sure you want to change the base?
chore(infra): remove --platform build flags from Dockerfiles #2242
Conversation
WalkthroughMultiple Dockerfiles were changed to use --platform=${TARGETPLATFORM} instead of --platform=${BUILDPLATFORM} in FROM lines. The controlplane Dockerfile also added runtime settings (ENV NODE_ENV=production), copied runtime artifacts (including dbmate), set CMD, and EXPOSE 3001. Minor EOF newline fixes applied. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @joornby-angel, thanks for the PR! Good catch with the final images being set to BUILDPLATFORM, we can confirm internally that this is an issue. This PR also unsets the option on the builder stages, which is incorrect. If you can fix this up and rebase/merge in main that'd be great, feel free to re-request my review when that's done.
| @@ -1,4 +1,4 @@ | |||
| FROM --platform=${BUILDPLATFORM} node:lts AS builder | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option should be set for builders, it's what allows the cross compilation to be fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a little more reading. I'll modify the final host image platform to make sure the runtime looks right. While I do that, do you know:
If the GO compiler needs to be run through the target platform?I did a little more reading on this, and decided it's not a concern based on the GO compilation commands in the docker images.- If there are any node modules that pull in architecture dependent native binaries?
@endigma Thanks for getting back to me. I updated the PR to explicitly specify the target platform in the final stage so that the image itself documents the intent. Let me know what else you want changed. |
|
@endigma @miklosbarabas Is there anything else I need to do with this PR to get this moving forward? |
|
@endigma @miklosbarabas Anything else I can do to move this along? |
LGTM, @endigma WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we'll likely need to make our own PR on this commit to make it run CI fully though.
Yeah, that doesn't bother me. I'll be glad to help chase down any issues if some crop up, jus let me know. |
Summary by CodeRabbit
Chores
Refactor
Checklist