Skip to content

Remove tox #2135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 20, 2025
Merged

Remove tox #2135

merged 1 commit into from
Aug 20, 2025

Conversation

ulgens
Copy link
Member

@ulgens ulgens commented Jul 22, 2025 •
edited
Loading

tobiasmcnulty
tobiasmcnulty reviewed Jul 26, 2025
View reviewed changes
Copy link
Member

@tobiasmcnulty tobiasmcnulty left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up! I have not tested, but I think removing tox will help simplify things. One nitpick.

@tobiasmcnulty tobiasmcnulty mentioned this pull request Jul 26, 2025
Closed
@ulgens ulgens self-assigned this Jul 27, 2025
@ulgens ulgens marked this pull request as ready for review July 28, 2025 08:51
@ulgens ulgens requested review from bmispelon and a team July 28, 2025 08:51
@ulgens ulgens mentioned this pull request Jul 29, 2025
Open
bmispelon
bmispelon reviewed Aug 8, 2025
View reviewed changes
.github/workflows/tests.yml Outdated
@@ -38,7 +32,8 @@ jobs:
python-version: "${{ matrix.python-version }}"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that going to be broken now that you've removed the version matrix?

Copy link
Member Author

Choose a reason for hiding this comment

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

The python-version input is not set. The version of Python currently in PATH will be used.

It seems Github was automatically picking up Python 3.12.3 and doesn't fail, and I missed that.

I pushed a fix to specify 3.12 explicitly.

bmispelon
bmispelon requested changes Aug 8, 2025
View reviewed changes
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

I've read through the comments and I think we're all mostly in agreement: tox can go.

Regarding the version matrix, I think it can be safely removed now that:

  • Both the websiste and Trac are dockerized (they can run independent python versions as long as they run django versions that are database-compatible)
  • We will switch to a preview server (see #1922 and #2153)

There are still mentions of the version matrix in the yaml file though, I think those should be removed. Come to think of it, what will be the new single source of truth when it comes to the Python version if we remove the matrix?

@ulgens ulgens force-pushed the remove-tox branch 3 times, most recently from e38e7ba to f2293d6 Compare August 9, 2025 12:53
@ulgens
Copy link
Member Author

ulgens commented Aug 9, 2025

I think those should be removed.

You are right, I pushed an update.

what will be the new single source of truth when it comes to the Python version if we remove the matrix?

For the test pipeline, we can continue to use the same python-version argument with a specific Python version as the input. In case of a migration to uv / pyproject.toml (#1901) uv Python installer has the same argument: https://github.com/astral-sh/setup-uv?tab=readme-ov-file#python-version

@ulgens ulgens requested a review from bmispelon August 9, 2025 12:57
bmispelon
bmispelon requested changes Aug 15, 2025
View reviewed changes
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

There is still a reference to matrix.python-version which should be removed:

COVERALLS_FLAG_NAME: "${{ matrix.python-version }}"

(line 64)

Other than that I think this is ready to merge, nice work 👍🏻

@adamzap
Copy link
Member

adamzap commented Aug 15, 2025

Do we still use Coveralls? https://coveralls.io/github/django/djangoproject.com

@ulgens
Copy link
Member Author

ulgens commented Aug 16, 2025

There is still a reference to matrix.python-version which should be removed:

Yep. Pushed another fix.

@ulgens ulgens requested a review from bmispelon August 16, 2025 09:59
@bmispelon
Copy link
Member

Do we still use Coveralls? https://coveralls.io/github/django/djangoproject.com

No idea 🤷🏻 I know I don't, but I don't know when that was added and/or why. It's possible it doesn't make sense anymore.

@ulgens ulgens mentioned this pull request Aug 19, 2025
Open
@ulgens ulgens requested a review from a team August 19, 2025 18:47
SaptakS
SaptakS approved these changes Aug 20, 2025
View reviewed changes
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@SaptakS SaptakS merged commit 33cbad9 into django:main Aug 20, 2025
5 checks passed
@ulgens ulgens deleted the remove-tox branch August 21, 2025 07:19
@ulgens ulgens mentioned this pull request Aug 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasmcnulty tobiasmcnulty tobiasmcnulty left review comments

@bmispelon bmispelon bmispelon approved these changes

@SaptakS SaptakS SaptakS approved these changes

Assignees

@ulgens ulgens

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ulgens @adamzap @bmispelon @tobiasmcnulty @SaptakS