-
Notifications
You must be signed in to change notification settings - Fork 39
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
Update all docker-images to Debian Bookworm (latest stable) #2723
base: master
Are you sure you want to change the base?
Conversation
73cf615
to
fcd9438
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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 mostly good to me, but I doubt we need the extra virtualenv complication if we switch from "pure" Debian to the "python:*-bookworm" images.
requirements/base.txt
Outdated
psycopg2 # requires libpq to build | ||
IPy==1.01 | ||
pyaml | ||
|
||
twisted>=20.0.0,<21 | ||
twisted>=20.0.0 |
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'm sure you have a good reason for changing these requirements, but I would prefer it if that reason was stated more clearly in the commit log, for posterity. Since these affect more than just the dev images, a separate commit would be best, methinks..
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.
ENV VIRTUAL_ENV=/opt/venv | ||
RUN python3 -m venv $VIRTUAL_ENV | ||
ENV PATH="$VIRTUAL_ENV/bin:$PATH" |
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.
Actually, I don't think you need any of this virtualenv f**kery when working from the python images at Dockerhub. They don't use the Debian-provided python/pip, so PEP 668 doesn't seem to apply here.
In fact, I can't get NAV to start properly in this bookworm-based image unless I delete these three lines.
I'd be glad to see this go, as I don't see the point of mixing virtualenvs into containers (unless you, for some silly reason, actually need multiples of them within a single container)
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
This will likely be stuck in limbo until #2788 is complete. |
0a8b47b
to
83f87b8
Compare
23c6cc3
to
407a23f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2723 +/- ##
=======================================
Coverage 56.69% 56.69%
=======================================
Files 602 602
Lines 43971 43971
=======================================
Hits 24931 24931
Misses 19040 19040 ☔ View full report in Codecov by Sentry. |
f9f47cb
to
5222e0d
Compare
Quality Gate passedIssues Measures |
Bookworm does not allow distro pip to install anything in the system tree (/usr). See PEP 668 and https://pythonspeed.com/articles/externally-managed-environment-pep-668/ More importantly: install python stuff into a virtualenv, mirroring how it is done in production. See https://pythonspeed.com/articles/activate-virtualenv-dockerfile/ The howto above doesn't use sudo. We do, so note the extra fun in the shell scripts. (Sudo does not by default preserve the caller's path.)
Closes #2695