-
Notifications
You must be signed in to change notification settings - Fork 27
fix: update dockerfile to use virtualenv and python 3.8 #468
Conversation
6221e70
to
575827d
Compare
@@ -85,6 +85,7 @@ upgrade: piptools $(COMMON_CONSTRAINTS_TXT) ## re-compile requirements .txt fi | |||
mv requirements/test.tmp requirements/test.txt | |||
|
|||
piptools: | |||
pip install -r requirements/pip.txt |
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.
@ohnickmoy Can you describe why this was needed and/or how pip and setuptools was installed before this addition in places like devstack or production?
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.
my rationale is based on what was described here: https://github.com/openedx/edx-notes-api/pull/206/files#r499761008 and here: https://github.com/edx/demographics/pull/69/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R36
I believe it's to pin pip and setup tools to a particular version
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 compared this Dockerfile to the one in https://github.com/openedx/edx-notes-api/blob/master/Dockerfile and think all these changes make sense. It could be worth getting someone from #masters-cosmonauts to also review it before merging though, just in case they see something I miss.
P.S. please update the PR description.
RUN pip install -r requirements/pip.txt | ||
RUN pip install -r requirements/production.txt |
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.
Could you revert these lines? It looks like they do (very very nearly) the same thing, from reading the Makefile. (Also helps if the stuff called "production" stays the thing we do in production, and reverting these lines is the easiest way I can see of making sure that's still true in the future.)
We are closing this pull request because Registar is deprecated. If you work with 2U and you still want to make this change, you could propose it on 2U's fork of Registar, but the contribution will not be part of the Open edX project. |
Description
TODO: Include a detailed description of the changes in the body of the PR
Part of PSRE-1460 and the work to containerize registrar. Adds lines to run setup within a virtual environment
Updates the registrar docker file to use a virtualenv and python 3.8 instead of 3.5