-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
PLAT-2060 Use pip-tools to manage requirements files (take 2) #17927
Conversation
e2171f8
to
c22196e
Compare
django-config-models==0.1.8 # Configuration models for Django allowing config management with auditing | ||
django-cors-headers==2.1.0 # Used to allow to configure CORS headers for cross-domain requests | ||
django-countries==4.6.1 # Country data for Django forms and model fields | ||
django-fernet-fields # via edx-enterprise (should be added to its setup.py) |
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.
@jmbowman We had a little discussion about this at https://github.com/edx/edx-enterprise/pull/303/files#r180804786 -- can you comment on whether we need to add it to edx-enterprise and erase it from here, or...?
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.
django-fernet-fields
is the edX-recommended package to use for storing encrypted values in the database. We added it to edx-platform requirements, so that future use cases wouldn't have to.
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 thinking was that because django-fernet-fields is only used by edx-enterprise, it should be listed as a dependency in the install_requires
field of its setup.py
(which is currently derived from the content of requirements/base.txt
). With the new pip-tools configuration in edx-platform, this dependency gets auto-detected by pip-compile and stored in the generated requirements files with a comment like "via edx-enterprise".
My concern about not having it listed as a dependency of edx-enterprise is that we're then counting on a line in edx-platform that says "we need this because edx-enterprise uses it"; if edx-enterprise ever changes such that it needs a different version or doesn't need it at all anymore, somebody has to remember to go change the appropriate lines in edx-platform requirements files. If that information is in the edx-enterprise setup.py
, it will get automatically updated in edx-platform when make upgrade
is run to update the edx-enterprise verion.
Note that this PR was reverted because of issues building sandboxes from scratch (we have some bizarre stuff in our local package installations), but I'm actively fixing that and hope to have it merged again in a day or two.
c22196e
to
63920ea
Compare
Your PR has finished running tests. |
63920ea
to
f04b356
Compare
Your PR has finished running tests. |
f04b356
to
5769585
Compare
Your PR has finished running tests. |
This reverts commit a7fa0c2.
5769585
to
9ca9aa4
Compare
Your PR has finished running tests. |
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, April 16, 2018. |
EdX Release Notice: This PR has been deployed to the production environment. |
EdX Release Notice: This PR has been rolled back from the production environment. |
This splits requirements into high-level direct dependency files managed by hand, and fully detailed requirements files generated by pip-compile. The detailed files are all recreated by the "make upgrade" target. Points worth noting:
pre.txt
requirements file; those should probably be merged before this.pip install
with them as normal, we just don't gain the benefit of automatically uninstalling any packages which were removed from the requirements.Packages which were deliberately upgraded:
Packages with recent releases which hadn't been pinned: