-
Notifications
You must be signed in to change notification settings - Fork 19
feat: modernize Python tooling (pyproject.toml + uv) #1015
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
e40a74e
78726ed
0296dc7
2a4390b
1ab7bc1
1ed7a71
de63148
15fc242
61b1e4b
6bd8356
f7a6546
b2c3f92
d7ebd16
bac14a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,32 +5,58 @@ on: | |
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
| workflow_call: | ||
|
|
||
| jobs: | ||
| tests: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.12"] | ||
| django-version: ["pinned", "5.2"] | ||
| toxenv: | ||
| - "py312-django42" | ||
| - "py312-django52" | ||
| - "translations" | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | ||
| with: | ||
| enable-cache: true | ||
| python-version: '3.12' | ||
|
|
||
| - name: Install Dependencies | ||
| run: uv sync --group ci | ||
|
|
||
| - name: Run Tests | ||
| env: | ||
| TOXENV: ${{ matrix.toxenv }} | ||
| run: uv run tox | ||
|
|
||
| - name: Codecov | ||
| if: startsWith(matrix.toxenv, 'py312-django') | ||
| run: uv run codecov | ||
|
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. This condition does not pin to a specific if: startsWith(matrix.toxenv, 'py312-django') && matrix.python-version == '3.12'
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. Fixed -- added |
||
|
|
||
| quality: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| toxenv: | ||
| - "quality" | ||
| - "pii_check" | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| architecture: x64 | ||
| - name: Install requirements | ||
| run: make requirements | ||
| - name: Upgrade packages | ||
| run: | | ||
| pip install -U pip wheel -c requirements/constraints.txt | ||
| if [[ "${{ matrix.django-version }}" != "pinned" ]]; then | ||
| pip install "django~=${{ matrix.django-version }}.0" | ||
| pip check # fail if this test-reqs/Django combination is broken | ||
| fi | ||
| - name: Validate translations | ||
| run: make validate_translations | ||
| - name: Run tests | ||
| run: make test | ||
| - name: Run style | ||
| run: make style isort_check pii_check check_keywords | ||
| enable-cache: true | ||
| python-version: '3.12' | ||
|
|
||
| - name: Install Dependencies | ||
| run: uv sync --group ci | ||
|
|
||
| - name: Run Checks | ||
| env: | ||
| TOXENV: ${{ matrix.toxenv }} | ||
| run: uv run tox | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,12 +25,15 @@ jobs: | |
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| ref: ${{ env.target_branch }} | ||
|
|
||
| - name: setup python | ||
| uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 | ||
|
|
||
|
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.
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. Fixed -- removed the redundant |
||
| - name: make upgrade | ||
| run: | | ||
| cd $GITHUB_WORKSPACE | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,13 @@ | ||
| .DEFAULT_GOAL := help | ||
|
|
||
| .PHONY: help clean piptools requirements ci_requirements dev_requirements \ | ||
| .PHONY: help clean compile-requirements requirements ci_requirements dev_requirements \ | ||
| validation_requirements doc_requirements production-requirements static shell \ | ||
| test coverage isort_check isort style lint quality pii_check validate \ | ||
| migrate html_coverage upgrade extract_translation dummy_translations \ | ||
| compile_translations fake_translations pull_translations \ | ||
| push_translations start-devstack open-devstack pkg-devstack \ | ||
| detect_changed_source_translations validate_translations check_keywords | ||
|
|
||
| COMMON_CONSTRAINTS_TXT=requirements/common_constraints.txt | ||
| .PHONY: $(COMMON_CONSTRAINTS_TXT) | ||
| $(COMMON_CONSTRAINTS_TXT): | ||
| wget -O "$(@)" https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt || touch "$(@)" | ||
|
|
||
| define BROWSER_PYSCRIPT | ||
| import os, webbrowser, sys | ||
| try: | ||
|
|
@@ -36,25 +31,24 @@ clean: ## delete generated byte code and coverage reports | |
| rm -rf assets | ||
| rm -rf pii_report | ||
|
|
||
| piptools: ## install pinned version of pip-compile and pip-sync | ||
| pip install -U 'pip<25.3' | ||
| pip install -r requirements/pip-tools.txt | ||
| compile-requirements: ## generate the uv.lock file without upgrading packages | ||
| uv lock | ||
|
|
||
| requirements: piptools dev_requirements ## sync to default requirements | ||
| requirements: dev_requirements ## sync to default requirements | ||
|
|
||
| ci_requirements: validation_requirements ## sync to requirements needed for CI checks | ||
|
|
||
| dev_requirements: ## sync to requirements for local development | ||
| pip-sync -q requirements/dev.txt | ||
| uv sync --group dev | ||
|
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. The
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. Confirmed already fixed -- |
||
|
|
||
| validation_requirements: ## sync to requirements for testing & code quality checking | ||
| pip-sync -q requirements/validation.txt | ||
| uv sync --group quality | ||
|
|
||
| doc_requirements: | ||
| pip-sync -q requirements/doc.txt | ||
| uv sync --group doc | ||
|
|
||
| production-requirements: piptools ## install requirements for production | ||
| pip-sync -q requirements/production.txt | ||
| production-requirements: ## install requirements for production | ||
| uv sync --group production | ||
|
|
||
| static: ## generate static files | ||
| python manage.py collectstatic --noinput | ||
|
|
@@ -63,24 +57,24 @@ shell: ## run Django shell | |
| python manage.py shell | ||
|
|
||
| test: clean ## run tests and generate coverage report | ||
| pytest --ds=enterprise_access.settings.test | ||
| uv run pytest --ds=enterprise_access.settings.test | ||
|
|
||
| # To be run from CI context | ||
| coverage: clean | ||
| pytest --cov-report html | ||
| uv run pytest --cov-report html | ||
| $(BROWSER) htmlcov/index.html | ||
|
|
||
| isort_check: ## check that isort has been run | ||
| isort --check-only enterprise_access/ | ||
| uv run isort --check-only enterprise_access/ | ||
|
|
||
| isort: ## run isort to sort imports in all Python files | ||
| isort --atomic enterprise_access/ | ||
| uv run isort --atomic enterprise_access/ | ||
|
|
||
| style: ## run Python style checker | ||
| pycodestyle enterprise_access manage.py | ||
| uv run pycodestyle enterprise_access manage.py | ||
|
|
||
| lint: ## run Python code linting | ||
| pylint --rcfile=pylintrc enterprise_access/apps/customer_billing *.py | ||
| uv run pylint --rcfile=pylintrc enterprise_access/apps/customer_billing *.py | ||
|
|
||
| quality: style isort_check lint ## check code style and import sorting, then lint | ||
| @echo "\e[32mQuality tests passed!\e[0m" | ||
|
|
@@ -106,33 +100,9 @@ html_coverage: ## generate and view HTML coverage report | |
| subsidy_client_local: # re-install edx-enterprise-subsidy-client from local code | ||
| pip uninstall -y edx-enterprise-subsidy-client && pip install -e /edx/src/edx-enterprise-subsidy-client/ && pip freeze | grep subsidy-client | ||
|
|
||
| COMMON_CONSTRAINTS_TXT=requirements/common_constraints.txt | ||
| .PHONY: $(COMMON_CONSTRAINTS_TXT) | ||
| $(COMMON_CONSTRAINTS_TXT): | ||
| wget -O "$(@)" https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt || touch "$(@)" | ||
| echo "$(COMMON_CONSTRAINTS_TEMP_COMMENT)" | cat - $(@) > temp && mv temp $(@) | ||
|
|
||
| upgrade: export CUSTOM_COMPILE_COMMAND=make upgrade | ||
| upgrade: piptools $(COMMON_CONSTRAINTS_TXT) ## update the requirements/*.txt files with the latest packages satisfying requirements/*.in | ||
| sed 's/django-simple-history==3.0.0//g' requirements/common_constraints.txt > requirements/common_constraints.tmp | ||
| mv requirements/common_constraints.tmp requirements/common_constraints.txt | ||
| # Make sure to compile files after any other files they include! | ||
| sed 's/Django<4.0//g' requirements/common_constraints.txt > tmp_con; cat tmp_con > requirements/common_constraints.txt; rm tmp_con | ||
| pip-compile --allow-unsafe --rebuild --upgrade -o requirements/pip.txt requirements/pip.in | ||
| pip-compile --upgrade -o requirements/pip-tools.txt requirements/pip-tools.in | ||
| pip install -qr requirements/pip.txt | ||
| pip install -qr requirements/pip-tools.txt | ||
| pip-compile --upgrade -o requirements/base.txt requirements/base.in | ||
| pip-compile --upgrade -o requirements/test.txt requirements/test.in | ||
| pip-compile --upgrade -o requirements/doc.txt requirements/doc.in | ||
| pip-compile --upgrade -o requirements/quality.txt requirements/quality.in | ||
| pip-compile --upgrade -o requirements/validation.txt requirements/validation.in | ||
| pip-compile --upgrade -o requirements/dev.txt requirements/dev.in | ||
| pip-compile --upgrade -o requirements/production.txt requirements/production.in | ||
| # Let tox control the Django version for tests | ||
| grep -e "^django==" requirements/base.txt > requirements/django.txt | ||
| sed '/^[dD]jango==/d' requirements/test.txt > requirements/test.tmp | ||
| mv requirements/test.tmp requirements/test.txt | ||
| upgrade: ## upgrade all packages in uv.lock and sync constraints from edx-lint | ||
| uv run --with edx-lint edx_lint write_uv_constraints pyproject.toml | ||
| uv lock --upgrade | ||
|
|
||
| extract_translations: ## extract strings to be translated, outputting .mo files | ||
| python manage.py makemessages -l en -v1 -d django | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,6 @@ | ||
| """ | ||
| This will make sure the app is always imported when | ||
| Django starts so that shared_task will use this app. | ||
| Make sure the app is always imported when Django starts so that shared_task will use this app. | ||
| """ | ||
| from .celery import app as celery_app | ||
|
|
||
| __all__ = ('celery_app',) | ||
|
|
||
|
|
||
| __version__ = '1.0.1' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,7 @@ | |
| from rest_framework import status | ||
| from rest_framework.reverse import reverse | ||
|
|
||
| from enterprise_access.apps.core.constants import ( | ||
| ALL_ACCESS_CONTEXT, | ||
| SYSTEM_ENTERPRISE_LEARNER_ROLE, | ||
| SYSTEM_ENTERPRISE_OPERATOR_ROLE, | ||
| SYSTEM_ENTERPRISE_PROVISIONING_ADMIN_ROLE | ||
| ) | ||
| from enterprise_access.apps.core.constants import SYSTEM_ENTERPRISE_LEARNER_ROLE | ||
| from enterprise_access.apps.core.tests.factories import UserFactory | ||
| from enterprise_access.apps.customer_billing.constants import CheckoutIntentState | ||
| from enterprise_access.apps.customer_billing.models import CheckoutIntent | ||
|
|
@@ -682,13 +677,13 @@ def test_patch_non_embargoed_country_succeeds(self): | |
|
|
||
| def test_list_with_staff_permission_returns_all_records(self): | ||
| """Test that users with `is_staff=True` can see all checkout intents.""" | ||
| staff_user = self.create_user( | ||
| self.create_user( | ||
| username='a-staff-user', password='password', is_staff=True, | ||
| ) | ||
| self.set_jwt_cookie([{ | ||
| 'system_wide_role': SYSTEM_ENTERPRISE_LEARNER_ROLE, | ||
|
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. why are we doing this change?
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. This is a behavior-preserving cleanup -- |
||
| 'context': str(uuid.uuid4()), | ||
| }], user=staff_user) | ||
| }]) | ||
|
|
||
| response = self.client.get(self.list_url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
|
|
@@ -706,13 +701,13 @@ def test_list_with_staff_permission_returns_all_records(self): | |
|
|
||
| def test_retrieve_other_users_record_with_staff_permission(self): | ||
| """Test that users with staff permission can retrieve other users' records.""" | ||
| staff_user = self.create_user( | ||
| self.create_user( | ||
| username='a-staff-user', password='password', is_staff=True, | ||
| ) | ||
| self.set_jwt_cookie([{ | ||
| 'system_wide_role': SYSTEM_ENTERPRISE_LEARNER_ROLE, | ||
|
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. why are we doing this change?
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. This is a behavior-preserving cleanup -- |
||
| 'context': str(uuid.uuid4()), | ||
| }], user=staff_user) | ||
| }]) | ||
|
|
||
| # Try to access user_3's checkout intent (should succeed with permission) | ||
| response = self.client.get(self.detail_url_3) | ||
|
|
||
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.
These matrix values change the check names from the old
tests (3.12, pinned)format totests (py312-django42),tests (py312-django52),tests (translations). Iftests (3.12, pinned)is currently a required status check in branch protection, all future PRs will be blocked after this merges. Please confirm with a repo admin that branch protection rules are updated (or were never set to the old name) before merging. The currently failingtests (3.12, pinned)check on this PR likely confirms it was a required check.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.
perhaps
tests (3.12, pinned)are not running in the PR because of it.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.
Checked directly via the GitHub API (
repos/.../branches/main/protectionand.../rulesets) -- this repo has no branch protection rules or rulesets configured, so there's no required status check that could be broken by the renamed checks. Confirmed, not just assumed.