-
Notifications
You must be signed in to change notification settings - Fork 14
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
DM-41366: Add Docker image build for butler server #900
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #900 +/- ##
==========================================
- Coverage 87.68% 87.67% -0.01%
==========================================
Files 276 277 +1
Lines 36326 36338 +12
Branches 7581 7583 +2
==========================================
+ Hits 31851 31858 +7
- Misses 3313 3318 +5
Partials 1162 1162
☔ View full report in Codecov by Sentry. |
7b3d8c7
to
403d76f
Compare
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.
Looks good. It would be nice if the github action could run a simple test to check that the code can be loaded. eg by running something like:
$ butler create tmp
$ butler config-dump tmp
Dockerfile
Outdated
|
||
COPY . /workdir | ||
WORKDIR /workdir | ||
RUN pip install --no-cache-dir . |
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.
Maybe add --no-deps
here so it doesn't try again to install all the dependencies that we've installed via requirements.txt
above? Historically we have had some weird things going on with version number matches if requirements.txt is pointing to git repos and pip is using pyproject.toml
dependencies from PyPI.
compose.yml
Outdated
ports: | ||
- "8080:8080" | ||
environment: | ||
BUTLER_SERVER_BUTLER_ROOT: "/butler_root" |
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 am pondering whether BUTLER_ROOT is the right term now. This is really the URI to the butler configuration (with or without the butler.yaml
).
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.
renamed BUTLER_SERVER_CONFIG_URI
@@ -0,0 +1,2 @@ | |||
uvicorn | |||
psycopg2 |
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.
In theory pip install .[postgres]
should pick up this dependency (it's listed as an optional dep in the pyproject.toml), but not if I've just told you to use --no-deps
for that part above...
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 dependencies actually want to be installed in a separate step from the pip install of the application itself -- if the requirements.txt files don't change, Docker can cache the intermediate dependency image and save a few minutes on the next build.
Also the requirements will need to be pinned in future so we won't want pyproject.toml involved.
796f95e
to
84a2dc8
Compare
Checklist
doc/changes