Skip to content
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

🧪 Only run "lowest" & "supported" pip in tox for PR/push @ CI #2142

Merged
4 commits merged into from
Dec 16, 2024

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Dec 4, 2024

This PR implements "supported" and "lowest" (renamed from "previous") envs in tox. It then integrates both into the CI runs triggered by push and pull_request events.
It drops running the tests against "latest" and "main" envs in regular PRs, keeping them in
the matrix executed on cron.

This improves reproducibility of the CI that will unblock contribution flows.

It additionally fixes reusable workflow conditionals relying on ${{ github.job_workflow_sha }} that are broken within GitHub.

Refs:

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

This better reflects that the pip version being pulled in is the
lowest tested/supported.
This is meant to be separate from "latest" and is pinned to the known
working released version of pip. It will run on merges to `main` and
in pull requests. And the cron runs will additionally test the latest
version on PyPI and the `main` branch of the pip repo.
@webknjaz webknjaz added bug Something is not working refactor Refactoring code tests Testing and related things maintenance Related to maintenance processes pip Related to pip dependency Related to a dependency ci Related to continuous integration tasks labels Dec 4, 2024
@webknjaz webknjaz requested review from hugovk and chrysle December 4, 2024 16:06
@webknjaz webknjaz self-assigned this Dec 4, 2024
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

This PR implements "supported" and "minimum" (renamed from "previous") envs in tox. It then integrates both into the CI runs triggered by push and pull_request events.

Looks like "minimum" is actually "lowest" in the code, please could you rename one or the other so the PR title/description matches the code?

Comment on lines +4 to +6
py{38,39,310,311,312,py3}-pip{supported,lowest,latest,main}-coverage
pip{supported,lowest,latest,main}-coverage
pip{supported,lowest,latest,main}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
py{38,39,310,311,312,py3}-pip{supported,lowest,latest,main}-coverage
pip{supported,lowest,latest,main}-coverage
pip{supported,lowest,latest,main}
py{38,39,310,311,312,py3}-pip-{supported,lowest,latest,main}-coverage
pip-{supported,lowest,latest,main}-coverage
pip-{supported,lowest,latest,main}

Could we add dashes (and of course elsewhere where referenced) to improve readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Later. See the other comment.

Comment on lines +17 to +18
pipsupported: pip==24.2
piplowest: pip==22.2.*
Copy link
Member

Choose a reason for hiding this comment

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

What does "supported" actually mean here?

pyproject.toml requires pip >= 22.2, so that maps neatly onto piplowest, but the this lowest is also supported.

Is "pipsupported" the highest version we claim to support?

Like, there's a pip 24.3, but we don't claim to support that (yet)?

If so, would piphighest be clearer, and mirror piplowest better?

(and with dashes for readability)

Copy link
Member Author

Choose a reason for hiding this comment

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

What does "supported" actually mean here?

pyproject.toml requires pip >= 22.2, so that maps neatly onto piplowest, but the this lowest is also supported.

Is "pipsupported" the highest version we claim to support?

Like, there's a pip 24.3, but we don't claim to support that (yet)?

Yes, kind of. I just wanted to have something that is pinned in CI. So after this effort, when a PR fails the CI, it's clear that it's not related to something that said PR changed. I don't want to think too much about this. The immediate goal is to unblock development, and it can be refined later.

If so, would piphighest be clearer, and mirror piplowest better?

Maybe, I was considering it. But in order to avoid spending a lot of time overthinking, I've gone with "supported" as "something we run in CI".

(and with dashes for readability)

I was reminded recently that tox makes dash-separated chunks factors, which might not be desired. I thought about it promptly and decided that it's not worthy of being in the scope of this PR and blocking it. But it could be debated separately, once this is complete. I'm currently blocked on getting #2106 to a working state (plus have some work to do in pypi-publish, which is mostly what's distracting me from this PR).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, sounds good. Let's check this again when the other things are in place 👍

@webknjaz webknjaz changed the title 🧪 Only run "minimum" & "supported" pip in tox for PR/push @ CI 🧪 Only run "lowest" & "supported" pip in tox for PR/push @ CI Dec 8, 2024
webknjaz added a commit to webknjaz/pip-tools that referenced this pull request Dec 12, 2024
webknjaz added a commit to webknjaz/pip-tools that referenced this pull request Dec 12, 2024
webknjaz added a commit to webknjaz/pip-tools that referenced this pull request Dec 16, 2024
@github-merge-queue github-merge-queue bot closed this pull request by merging all changes into jazzband:main in e604dec Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working ci Related to continuous integration tasks dependency Related to a dependency maintenance Related to maintenance processes pip Related to pip refactor Refactoring code tests Testing and related things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants