-
Notifications
You must be signed in to change notification settings - Fork 415
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
feat(build): Docker build improvements #4272
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4272 +/- ##
=======================================
Coverage 96.60% 96.60%
=======================================
Files 1195 1195
Lines 39099 39099
=======================================
Hits 37773 37773
Misses 1326 1326 ☔ View full report in Codecov by Sentry. |
Dockerfile
Outdated
ARG TARGETARCH | ||
RUN if [ "${TARGETARCH}" != "amd64" ]; then \ | ||
RUN pip uninstall -y pip && \ | ||
if [ "${TARGETARCH}" != "amd64" ]; then \ | ||
apt-get update && apt-get install -y libpq-dev && rm -rf /var/lib/apt/lists/*; fi; |
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 wonder if we still need to install these for arm?
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.
We might not psycopg/psycopg2#1545
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.
E2E passes on ARM without 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.
Approved with a question
@@ -30,4 +30,4 @@ serve: | |||
test: | |||
docker compose run frontend \ | |||
npx cross-env E2E_CONCURRENCY=${E2E_CONCURRENCY} npm run test -- $(opts) \ | |||
|| docker compose logs flagsmith-api | |||
|| (docker compose logs flagsmith-api; exit 1) |
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.
Why did we add this? How was this working before?
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.
docker compose logs flagsmith-api
exited 0, making failed E2E jobs green.
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
pip
from runtime layers to reduce attack surface (and have nicer Docker Scout reports).requests
to a non-yanked version.pip-tools
(and pip as transient dependency) from development dependencies.How did you test this code?
The builds are tested in CI.