Skip to content

build: use pinned Django version for tests - #29890

Merged
nedbat merged 1 commit into
masterfrom
nedbat/use-pinned-django
Apr 28, 2022
Merged

build: use pinned Django version for tests#29890
nedbat merged 1 commit into
masterfrom
nedbat/use-pinned-django

Conversation

@nedbat

@nedbat nedbat commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

The existing method of installing Django will continue to use 3.2.11 even though 3.2.12 is available. We ran into this on the maple.master branch when we upgraded the pin to 3.2.12, and saw that the tests were still using 3.2.11

I've tried to accommodate both "master" and "upgrading" workflows here with the Django matrix, but maybe there is a better way? Installing "Django==~4.0.0" won't work now because it conflicts with too many of our other requirements.

Note: I've made one change to the action just for temporary testing of this pull request.

@nedbat
nedbat requested review from jmbowman and mraarif February 8, 2022 16:38
@nedbat

nedbat commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

@BbrSofiane @cmltaWt0 FYI

@nedbat

nedbat commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

BTW, if we like the logic here, we can solve the 4.0 problem just by commenting out that part of the Django version matrix.

@nedbat
nedbat force-pushed the nedbat/use-pinned-django branch from bb3d017 to 1ced12c Compare February 8, 2022 20:28
@nedbat

nedbat commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Oh, also looks like "django-3.2" tests are required, but will no longer exist after this change, so that has to be adjusted as well.

@mraarif

mraarif commented Feb 18, 2022

Copy link
Copy Markdown
Contributor

doing pip install "django~=3.2" should install Django 3.2.12

@mraarif

mraarif commented Feb 18, 2022

Copy link
Copy Markdown
Contributor

doing pip install "django~=3.2" should install Django 3.2.12 and we can just do

sudo pip install "django~=${{ matrix.django-version }}"

@nedbat

nedbat commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

doing pip install "django~=3.2" should install Django 3.2.12 and we can just do

sudo pip install "django~=${{ matrix.django-version }}"

I'm not sure what you are suggesting? This would be instead of using django.txt ever?

I'm concerned about having two different mechanisms for installing Django: in production, we use the specific version pinned in requirements/edx/django.txt. In tests we wouldn't. That seems like a bad idea.

This issue came up because the pin on the Maple branch was mistakenly changed to a non-existent Django version. The tests passed. The code would have failed in production. I'm trying to ensure that the tests accurately test the code as it will run in production.

@nedbat
nedbat force-pushed the nedbat/use-pinned-django branch from 1ced12c to 349b2ce Compare February 24, 2022 12:36
@nedbat
nedbat force-pushed the nedbat/use-pinned-django branch from 349b2ce to 0582eb9 Compare April 13, 2022 17:57

@UsamaSadiq UsamaSadiq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested on local as well. The recommended approach works fine and I believe we can go ahead and upgrade this change.
For the required checks, we'll need to mark the new tests as required from the repo settings.

Comment thread .github/workflows/unit-tests.yml Outdated
Comment thread .github/workflows/unit-tests.yml Outdated
The logic here seems to work, but Django 4.0 won't install over our
other pinned requirements, so tests fail for Django 4.0.
@nedbat
nedbat force-pushed the nedbat/use-pinned-django branch from 0582eb9 to 99038b2 Compare April 28, 2022 08:52
@UsamaSadiq

Copy link
Copy Markdown
Member

We'll need to remove the old tests marked as required from the repo settings and mark the new tests as required to merge this PR.
@nedbat would you have access to do this? or should I create and SRE ticket for this?

@nedbat

nedbat commented Apr 28, 2022

Copy link
Copy Markdown
Contributor Author

I've removed the django-3.2 required checks, and will enable the -pinned checks once merged.

@nedbat
nedbat merged commit e7caec5 into master Apr 28, 2022
@nedbat
nedbat deleted the nedbat/use-pinned-django branch April 28, 2022 09:28
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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.

4 participants