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

Proposal: Dev & CI workflow improvement #1354

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

noirbizarre
Copy link
Member

@noirbizarre noirbizarre commented Feb 8, 2025

Description

This PR is a proposal improvement on the dev & CI workflows:

  • Use poethepoet as plugin to run tasks
  • Use the same tasks in CI
  • Use the same dependencies groups in CI
  • Only install required dependency groups in each CI jobs
  • Add tox support to easily test against all supported Python version
  • Removes redundant pre-push hooks as they already run on commit

On poetry install, poethepoet is installed locally as a plugin.
Tasks are exposed both as:

  • poetry sub commands
  • poe tasks

You can run:

poetry test
poe test

Tasks accept extra parameters, so it's now possible to run tests with queries:

poetry test -k bump

So are tox tests (run in parallel)

poetry test:all -k bump

To run the same thing as in GitHub action, run poetry ci

You can also preview the documentation with poetry doc

Available tasks

image

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

  • no GitHub actions regression but slighly faster jobs (less dependencies to install)
  • easier onboarding and contribution (all commands are discoverable and accept extra parameters)
  • easier contribution (all commands accept extra parameters to fit everyone)
  • less temptation to commit with --no-verify (pre-commit hooks are lighter and don't prevent push wip branches or in case of flapping test)

Steps to Test This Pull Request

  1. Checkout this PR
  2. poetry install
  3. List tasks with poetry list or poe (poetry run poe if you don't have it globally installed)
  4. Try any command

Additional context

Fix #724

Note

It makes possible to have the documentation screenshots easily generated on the fly in both GHA workflow and poetry doc command and so, not having commit them and to store them in the repo

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.55%. Comparing base (120d514) to head (6e542f0).
Report is 557 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
+ Coverage   97.33%   97.55%   +0.21%     
==========================================
  Files          42       55      +13     
  Lines        2104     2617     +513     
==========================================
+ Hits         2048     2553     +505     
- Misses         56       64       +8     
Flag Coverage Δ
unittests 97.55% <ø> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lee-W
Copy link
Member

Lee-W commented Feb 8, 2025

I love this! Would love to know how @woile before reviewing it. Another idea I had was to migrate to uv, but looking at this awesome PR. hmmm.. we probably should stay in poetry. Will try to take a deeper look

@woile
Copy link
Member

woile commented Feb 8, 2025

LGTM, I've also been interested in trying uv, but not sure if it's worth the effort 😅

@noirbizarre
Copy link
Member Author

Great, I am going to merge this one 👌🏼

I can try submitting a new PR for switching to uv next week, but I am not sure if it is worth it currently because uv is interesting because it's fast, but poetry 2.0 is also fast, we are talking about a few seconds on lock mostly.

Migrating should be simple:

So it'really straightforward now. If you are interested, I can do a PR.

However, we would lose Dependabot support until dependabot/dependabot-core#10478 is done

@noirbizarre noirbizarre merged commit 6920303 into commitizen-tools:master Feb 8, 2025
18 checks passed
@Lee-W
Copy link
Member

Lee-W commented Feb 8, 2025

Great, I am going to merge this one 👌🏼

I can try submitting a new PR for switching to uv next week, but I am not sure if it is worth it currently because uv is interesting because it's fast, but poetry 2.0 is also fast, we are talking about a few seconds on lock mostly.

Yep, we don't really need to. I mentioned it simply because Airflow is using it and I recently migrate my personal workflow to uv (still have some poetry part though).

Migrating should be simple:

* `uv` doesn't have "project management" facilities, it would still use `poe` for scripts ([which is supporting uv](https://poethepoet.natn.io/guides/without_poetry.html#usage-with-uv))

* Poetry dependency groups would be migrated to [PEP 735 dependency groups](https://peps.python.org/pep-0735/), which [`uv` supports](https://docs.astral.sh/uv/concepts/projects/dependencies/#dependency-groups)

The last time I check it was still draft and now it's final!

just notice there's a PR for pep 735 support
python-poetry/poetry-core#823

* publication would have to rely on another tool like `twine`

I guess we'll just use hatch if we're to change to uv

* GitHub action would rely on [`setup-uv`](https://docs.astral.sh/uv/guides/integration/github/)

So it'really straightforward now. If you are interested, I can do a PR.

However, we would lose Dependabot support until dependabot/dependabot-core#10478 is done

yep, this is one major downside.

I probably would say we don't really need to migrate to uv unless someone is really interested in making it happen.

PYPI_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
run: |
./scripts/publish
POETRY_HTTP_BASIC_PYPI_USERNAME: ${{ secrets.PYPI_USERNAME }}
Copy link
Member

Choose a reason for hiding this comment

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

We probably could migrate to trusted publisher in the future

@@ -99,8 +99,12 @@ version_scheme = "pep440"
[tool.poetry]
packages = [{ include = "commitizen" }, { include = "commitizen/py.typed" }]

[tool.poetry.requires-plugins]
"poethepoet" = ">=0.32.2"
Copy link
Member

Choose a reason for hiding this comment

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

The last time I tried this when testing project template generation, it worked weirdly. I probably should try it again. maybe it's fixed

]

[tool.tox]
requires = ["tox>=4.22"]
Copy link
Member

Choose a reason for hiding this comment

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

should we make it consistent with tox = ">4"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants