-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
added multiarch build #6577
added multiarch build #6577
Conversation
Hey @AndrewChubatiuk ! Thanks for the PR. We've tried a arm64 build on GHA recently and it runs super long - so long, in fact, that it hits some hidden 6 hour limit for GHA and does not finish. Would you mind testing this on your own repo and potentially making changes there before we consider merging this in? |
this change will run arm64 and amd64 image builds on separate machines, so it should improve build speed. okay, will test it in my fork |
Here's an example action that we already ran: https://github.com/getredash/redash/actions/runs/6546702269 I'm not sure how that was running so maybe we just did it wrong. :) |
@@ -1,7 +1,5 @@ | |||
FROM node:16.20.1-bookworm as frontend-builder | |||
|
|||
RUN npm install --global --force [email protected] |
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.
Could you explain the changes you're making to the Dockerfile here?
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.
Removed frontend build from Dockerfile as there are to many places, where same node dependencies are installed. Also there's no sense to build same platform independent frontend in many places
client/.babelrc
Outdated
@@ -4,7 +4,7 @@ | |||
"@babel/preset-env", | |||
{ | |||
"exclude": ["@babel/plugin-transform-async-to-generator", "@babel/plugin-transform-arrow-functions"], | |||
"corejs": "2", | |||
"corejs": "3", |
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 don't understand any of the JS dependency related changes; @eradman would you mind taking a look? Are we able to change these deps to enable arm64 images?
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.
reverted frontend changes, as they are built separately now
e5c84c7
to
0b64109
Compare
@guidopetri Finished doing my changes. you can check build time here There's lot's of work still, lot's of python packages which are either not supported or outdated, do not have arm64 wheels. Making them up to date can reduce build time as well Also node dependencies for cypress can be optimized, cause it also takes several minutes to install them inside container |
@guidopetri @eradman |
This change seems to check out for me. Triggering CI tests |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6577 +/- ##
=======================================
Coverage 61.89% 61.89%
=======================================
Files 158 158
Lines 12992 12992
Branches 1774 1774
=======================================
Hits 8042 8042
Misses 4673 4673
Partials 277 277 |
The code coverage report is not relavent. @guidopetri can you re-test a build with these changes on arm64? |
npm install --global --force [email protected] | ||
yarn cache clean && yarn --frozen-lockfile --network-concurrency 1 | ||
|
||
name: frontend |
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'm not 100% clear where this frontend
gets used in the build. Would you mind explaining? Does this also get pushed to Dockerhub as part of the preview
image?
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.
@guidopetri frontend is added to docker image during a build for each target arch and then first pushed to redash/preview and then copied from redash/preview to redash/redash
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.
How do we build this locally, then? docker build -t redash_test .
seems to not build the frontend
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'm unclear on what these changes are doing, and I think we need better documentation around this. How do we build things locally? What exactly is happening in the GHA build process? Why do we need to split up the frontend build?
|
Hmm, alright. I think we should separate this into two PRs then:
Does this make sense to you too? Or how do you think we should proceed? |
merged alternative PR in #6674 |
What type of PR is this?
Build multiarch docker images
Description
Added ability to create multiarch images
How is this tested?