Skip to content
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

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

aht007
Copy link
Member

@aht007 aht007 commented Sep 6, 2022

Description: Improve requirements file structure

JIRA: https://2u-internal.atlassian.net/browse/BOM-2483

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@aht007 aht007 marked this pull request as draft September 6, 2022 12:30
@aht007 aht007 force-pushed the aht007/BOM-2483-improve-req-structure branch from ee74c2a to cdea8e5 Compare September 23, 2022 12:48
@aht007 aht007 force-pushed the aht007/BOM-2483-improve-req-structure branch from 70cc16f to a756d80 Compare September 26, 2022 08:39
.gitignore Outdated
@@ -86,3 +86,6 @@ super-csv-env/

# test output
csv/

venv

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants