-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: improve requirements file structure #116
base: master
Are you sure you want to change the base?
Conversation
ee74c2a
to
cdea8e5
Compare
70cc16f
to
a756d80
Compare
.gitignore
Outdated
@@ -86,3 +86,6 @@ super-csv-env/ | |||
|
|||
# test output | |||
csv/ | |||
|
|||
venv |
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.
This should probably go under the "local virtualenvs" section above.
tox.ini
Outdated
sed -i.tmp '/^django==/d;/^vine==/d;/^amqp==/d;/^anyjson==/d' {toxinidir}/requirements/base.txt | ||
sed -i.tmp '/^djangorestframework==/d;/^billiard==/d' {toxinidir}/requirements/base.txt | ||
sed -i.tmp '/^celery==/d;/^kombu==/d' {toxinidir}/requirements/base.txt | ||
rm {toxinidir}/requirements/base.txt.tmp |
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.
This handles removing the constraints before running tox, but doesn't add them back afterwards. It would probably be better to make these changes in a new copy of base.txt (tox_temp.txt?), use that copy in the deps section below, and then remove the temp file afterwards. Should also add the temporary file to .gitignore in case tox gets interrupted and the file isn't properly removed.
So I think you'd want tox_temp.txt and test_layer.txt in the deps section instead of the test.txt convenience file, to avoid needing a temporary copy of that also (since it references base.txt explicitly).
Also, note that there's a bug in the existing code in that it never tests the exact versions of Celery, Django, etc. specified in the requirements files, it always goes for the latest release in the ranges specified here. I think Ned fixed this in edx-platform, but the new setup may require a slightly different solution than what he did there.
Description: Improve requirements file structure
JIRA: https://2u-internal.atlassian.net/browse/BOM-2483
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.