-
Notifications
You must be signed in to change notification settings - Fork 18
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
Use uv to install deps during docker build #761
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
=======================================
Coverage 68.28% 68.28%
=======================================
Files 45 45
Lines 4143 4143
=======================================
Hits 2829 2829
Misses 1314 1314
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 @danielhollas thanks for the PR. A few question on the changes, please see my comments.
@@ -38,7 +38,7 @@ jobs: | |||
# Also, they might have running or stopped containers, | |||
# which are not cleaned up by `docker system prun` | |||
- name: Reset docker state and cleanup artifacts 🗑️ | |||
if: ${{ inputs.platform != 'x86_64' }} | |||
if: ${{ startsWith(inputs.runsOn, 'ubuntu') }} |
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.
The new changes only support ubuntu
?
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 step of resetting Docker is only needed on our self-hosted arm64 runner. This if statement just ensures that this step is skipped run on the official x86 runner.
In a next PR I am going to remove this altogether since we want to stop using the self-hosted runner.
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.
Ah, made a mistake here, the if condition is inverted, it should be
if: ${{ startsWith(inputs.runsOn, 'ubuntu') }} | |
if: ${{ ! startsWith(inputs.runsOn, 'ubuntu') }} |
This is fixed in #764
@@ -1,4 +1,5 @@ | |||
# syntax=docker/dockerfile:1 | |||
FROM ghcr.io/astral-sh/uv:0.2.18 as uv |
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.
FROM
two images? And nothing is done for the first one?
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 first FROM is only used to access the uv binary. Please see the link to the uv documentation that I provided in a comment below, which is where this pattern comes from.
Essentially, this is to avoid the uv binary being in the final 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.
Thanks for the explanation and the excellent work!
This cuts down the build time by a full minute.