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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove tox #2135

wants to merge 1 commit into from

Conversation

ulgens
Copy link
Member

@ulgens ulgens commented Jul 22, 2025

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.

matrix:
# tox-gh-actions will only run the tox environments which match the currently
# running python-version. See [gh-actions] in tox.ini for the mapping.
python-version: ["3.12"]
Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to leave the matrix strategy in place, remove the comment about tox-gh-actions, and add a comment to say that new Python versions can be added here when upgrading to support both side by side.

Copy link
Member Author

@ulgens ulgens Jul 26, 2025

Choose a reason for hiding this comment

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

Hey @tobiasmcnulty , this was discussed in #1890 (comment) . Because we don't need to support multiple versions at the same deployment, Python version updates and their testing / QA processes can be handled in their own branches. I see no harm in keeping the matrix but I also don't see the benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, because of Trac, I don't think we will be able to update the Python version any time soon. This was one of the major points for discussions about moving away from Trac.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @ulgens, if I recall, upgrading the website separately from Trac is the benefit of the matrix strategy.

I'm not very familiar with the dependencies between the two repos, but perhaps we should add python3.11 to the matrix now, since that's what Trac is using? Maybe you have some insight on this @bmispelon?

When I added 3.12 support for the website, a few requirements had to remained pinned to the older versions I believe so as not to break the Trac integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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
@@ -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.

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
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.

3 participants