-
Notifications
You must be signed in to change notification settings - Fork 135
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
Split Docker Build to accept build args to optionally include swift support #741
Conversation
added dockerignore for slimer build context parametrized dockerfile
Codecov Report
@@ Coverage Diff @@
## master #741 +/- ##
=======================================
Coverage 79.90% 79.90%
=======================================
Files 15 15
Lines 2140 2140
Branches 311 311
=======================================
Hits 1710 1710
Misses 320 320
Partials 110 110 Continue to review full report at Codecov.
|
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, but I think it's going to break automation.
2 questions:
- How des this remove the setuptools dep? We use import from it
e.g. filter.py + storage.py import pkg_resources
- https://github.com/pypa/bandersnatch/blob/master/src/bandersnatch/filter.py#L7
- https://github.com/pypa/bandersnatch/blob/master/src/bandersnatch/storage.py#L25
- I feel this will need to pass this arg to the docker building GitHub action
- Seems you just need to add
buildargs: PY_VERSION=3.9
to https://github.com/pypa/bandersnatch/blob/master/.github/workflows/docker_upload.yml
@@ -1,28 +1,48 @@ | |||
FROM python:3.9 as base | |||
ARG PY_VERSION=3.9 |
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.
Sweet - 1 spot is way better!
1/ poor phrasing, sorry. it's just out from requirements and setup. Still needed tho. its presence triggered warning |
|
What warning? Seems weird to remove something we know is a dep. If you can't work it out can you leave it and open an issue and I'll try fix it.
What's this? Do I need to do something here? |
1/ issue is similar to pypa/pipenv#1417 (setuptools shouldn't be a dependency since you cannot install dependencies without it in the first place) the other matter is it seems I cannot push any changes in the .github repo. And you would need to evaluate my changes yourself, I'm afraid. It makes the image without swift support the standard one, and also build a variation with the optional swift support. |
Really, |
.github is checked out alright, but I can't push . Also, that Dockerfile doesn't build on raspberry pi ;( |
TIL. Ok, well I can fix that post merging this. Raspberry Pi has never been a target platform. File an issue with the errors + a repro and I or someone else will see if we can fix it. |
Please fix lint:
|
I'm looking to release this week so if you could fix the lints + put setuptools back so we can see what version we test with and what version each release / docker container uses that would be grand. |
Thanks. Will merge soon when I have time to fix the docker image building etc. right after merging. |
This still build and uploaded a docker image: Will try a PR with your suggested changes tho. |
added dockerignore for slimer build context
parametrized dockerfile
making swift support optional
nil/bandersnatch 3.9 9f730c999230 2 minutes ago 156MB
nil/bandersnatch 3.9-swift 91d50bd7103d 4 minutes ago 280MB
hopefully addressing #740 also