-
Notifications
You must be signed in to change notification settings - Fork 4.3k
build: Set Django version for tests more safely; drop support for non-GHA #31387
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,10 +24,8 @@ | |
| # a corresponding change to circle.yml, which is how the python | ||
| # prerequisites are installed for builds on circleci.com | ||
| toxenv = os.environ.get('TOXENV') | ||
| if toxenv and toxenv != 'quality-django32': | ||
| if toxenv and toxenv != 'quality': | ||
| PYTHON_REQ_FILES = ['requirements/edx/testing.txt'] | ||
| elif toxenv and toxenv == 'quality-django32': | ||
| PYTHON_REQ_FILES = ['requirements/edx/testing.txt', 'requirements/edx/django.txt'] | ||
| else: | ||
| PYTHON_REQ_FILES = ['requirements/edx/development.txt'] | ||
|
|
||
|
|
@@ -173,11 +171,7 @@ def python_prereqs_installation(): | |
| def pip_install_req_file(req_file): | ||
| """Pip install the requirements file.""" | ||
| pip_cmd = 'pip install -q --disable-pip-version-check --exists-action w' | ||
|
|
||
| if Env.PIP_SRC_DIR: | ||
| sh(f"{pip_cmd} -r {req_file} --src {Env.PIP_SRC_DIR}") | ||
| else: | ||
| sh(f"{pip_cmd} -r {req_file}") | ||
| sh(f"{pip_cmd} -r {req_file}") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Author's note] I believe that |
||
|
|
||
|
|
||
| @task | ||
|
|
@@ -313,8 +307,8 @@ def install_python_prereqs(): | |
| files_to_fingerprint.append(sysconfig.get_python_lib()) | ||
|
|
||
| # In a virtualenv, "-e installs" get put in a src directory. | ||
| if Env.PIP_SRC_DIR: | ||
| src_dir = Env.PIP_SRC_DIR | ||
| if Env.PIP_SRC: | ||
| src_dir = Env.PIP_SRC | ||
| else: | ||
| src_dir = os.path.join(sys.prefix, "src") | ||
| if os.path.isdir(src_dir): | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,6 +253,7 @@ dill==0.3.6 | |
| # via pylint | ||
| distlib==0.3.6 | ||
| # via virtualenv | ||
| django==3.2.16 | ||
| # via | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, kind of funny that we were ripping out the dependency but leaving the comments for it. |
||
| # -c requirements/edx/../common_constraints.txt | ||
| # -r requirements/edx/base.txt | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,10 +46,10 @@ COPY openedx/core/lib openedx/core/lib | |
| COPY lms lms | ||
| COPY cms cms | ||
| COPY requirements/pip.txt requirements/pip.txt | ||
| COPY requirements/edx/pip-tools.txt requirements/edx/pip-tools.txt | ||
| COPY requirements/edx/testing.txt requirements/edx/testing.txt | ||
| COPY requirements/edx/django.txt requirements/edx/django.txt | ||
| RUN pip install -r requirements/pip.txt && \ | ||
| pip install -r requirements/edx/testing.txt -r requirements/edx/django.txt | ||
| COPY Makefile Makefile | ||
| RUN make test-requirements | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you need to copy in the Makefile also to be able to run this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, seems likely! Adding that. :-) |
||
|
|
||
| FROM base as runner | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| [tox] | ||
| envlist = py38-django{32}, quality-django{32} | ||
| envlist = py38, quality | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we leave this as it was, but still make your change later in the file? I think the suffixes are harmless for now, and will be needed later when we start testing 4.0 and 4.1. And I like being more explicit about which Django version is being tested.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what benefit they would provide -- tox would no longer be controlling the Django version, and still wouldn't be once 4.x testing starts.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be for local testing of the 4.x upgrade. But I suppose it would be clearer to remove this for now and just add it back once it's actually implemented when we start that testing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for local testing what we'd want is just a branch where Django is upgraded to 4.x and tests are otherwise run as normal. |
||
|
|
||
| # This is needed to prevent the lms, cms, and openedx packages inside the "Open | ||
| # edX" package (defined in setup.py) from getting installed into site-packages | ||
|
|
@@ -64,9 +64,7 @@ passenv = | |
| XDIST_WORKER_KEY_NAME | ||
| XDIST_WORKER_SECURITY_GROUP | ||
| XDIST_WORKER_SUBNET | ||
| deps = | ||
| django32: -r requirements/edx/django.txt | ||
| -r requirements/edx/testing.txt | ||
| commands_pre = make test-requirements | ||
| whitelist_externals = | ||
| /bin/bash | ||
| /usr/bin/curl | ||
|
|
||
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.
[Author's note] I'm not sure if this was really correct in the first place -- shouldn't the quality checks be using dev deps (pylint will complain about missing django-debug-toolbar) and pytest be using just testing deps?
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.
We're looking into the feasibility of ripping out this paver code anyway; note that the GitHub Actions workflows already use pylint directly instead of going through paver.