-
Notifications
You must be signed in to change notification settings - Fork 89
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
chore: docker builds optimizations #1795
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
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 reckon we can still make the image sizes much smaller, but this is a good start. All node images come with npm
and yarn
pre-installed.
RUN apk add --no-cache \ | ||
python3 \ | ||
make \ | ||
g++ |
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.
node-gyp
requirements.
RUN apt update | ||
RUN apt install -y curl xz-utils python3 build-essential | ||
|
||
# version in curl is not the version used. Dependent on the last command | ||
RUN corepack enable | ||
RUN corepack prepare pnpm@7.25.1 --activate |
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.
Any particular reason we are still using pnpm@v7
instead of v8
?
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 guess we forgot to update it?
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.
Have you tried with pnpm v8?
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 have tried with pnpm v8 when I was running into the httpbis-digest-headers
issue. It should also work with v8 without any problems. As for the choice to stick with v7, I recall @mkurapov mentioning an edge case or an issue that was preventing us from upgrading to v8, but I'm not entirely sure about this.
02fc2e0
to
d3ff7b6
Compare
I'm currently running into some issues due to the |
We did publish the package, so we should just swap to using it: https://www.npmjs.com/package/httpbis-digest-headers |
We also need to use the NPM registry for rafiki/packages/auth/Dockerfile Line 23 in c815408
Do you want me to update the |
Yes please. Thank you! |
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 is fantastic! There is a lot of repetition across the dockerfiles. I don't know if we could somehow reduce that. But that was the reason I failed when trying to reduce the size so maybe we have to live with the repetition.
Thank you!
24db14d
Let me know if we should move to pnpm v8. |
This reverts commit 24db14d.
Yeah, let's move to pnpm v8 |
with: | ||
version: 7 |
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 can be removed since I've added pnpm's version in the root package.json
.
Lines 7 to 11 in 4a72018
"engines": { | |
"pnpm": "^8.7.4", | |
"node": "18" | |
}, | |
"packageManager": "pnpm@8.7.4", |
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.
Beautiful, works for me. You just need to resolve merge conflicts.
Changes proposed in this pull request
Context
Current progress:
Checklist
fixes #number