Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

fix: update dockerfile to use virtualenv and python 3.8 #468

Closed
wants to merge 1 commit into from

Conversation

ohnickmoy
Copy link
Contributor

@ohnickmoy ohnickmoy commented Apr 6, 2022

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

@ohnickmoy ohnickmoy requested a review from adzuci April 6, 2022 15:37
@ohnickmoy ohnickmoy force-pushed the nmoy/PSRE-1460_update_dockerfile branch from 6221e70 to 575827d Compare April 6, 2022 15:45
@adzuci adzuci requested review from matthugs, thomty and alangsto April 6, 2022 17:58
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@adzuci adzuci left a 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.

Comment on lines +44 to +45
RUN pip install -r requirements/pip.txt
RUN pip install -r requirements/production.txt
Copy link
Contributor

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.)

@kdmccormick
Copy link
Contributor

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.

@kdmccormick kdmccormick deleted the nmoy/PSRE-1460_update_dockerfile branch September 11, 2024 14:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants