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

ci: precommit checks for poetry TOML and lock #526

Merged
merged 5 commits into from
Mar 7, 2024
Merged

Conversation

d0choa
Copy link
Collaborator

@d0choa d0choa commented Mar 7, 2024

✨ Context

At the moment, nothing ensures that the project TOML is valid and in agreement with the poetry.lock in the repository. It would be great to ensure the sanity of these files, especially in the context of CI/CD and merging different branches with conflicting locks.

🛠 What does this PR implement

It implements the official poetry pre-commit hooks to perform poetry check and poetry lock --no-update in the files.

I ran poetry run pre-commit run --all-files to ensure checks are passing.

Update: poetry lock --no-update takes a bit of time. The CI throws a timeout, so I had to switch it off in CI. It would still run locally. If we see it slows down commits we will need to switch it off. It's very often skipped so it should not damage the overall experience.

Update 2: The pre-commit.ci now supports more modern python versions which tries to use by default. So I had to configure that it keeps using 3.10

🙈 Missing

  • I'm still not sure when this is triggered locally. It would be good to see in which scenarios the hook is triggered (not Skipped). Only in TOML changes, TOML and lock?

🚦 Before submitting

  • Do these changes cover one single feature (one change at a time)?
  • Did you read the contributor guideline?
  • Did you make sure to update the documentation with your changes?
  • Did you make sure there is no commented out code in this PR?
  • Did you follow conventional commits standards in PR title and commit messages?
  • Did you make sure the branch is up-to-date with the dev branch?
  • Did you write any new necessary tests?
  • Did you make sure the changes pass local tests (make test)?
  • Did you make sure the changes pass pre-commit rules (e.g poetry run pre-commit run --all-files)?

@github-actions github-actions bot added size-S and removed size-XS labels Mar 7, 2024
@d0choa d0choa marked this pull request as ready for review March 7, 2024 10:55
@@ -99,3 +102,10 @@ repos:
rev: 0.4.1
hooks:
- id: pydoclint

- repo: https://github.com/python-poetry/poetry
rev: "1.8.2"
Copy link
Collaborator Author

@d0choa d0choa Mar 7, 2024

Choose a reason for hiding this comment

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

I don't think we hardcode the poetry version anywhere. The devcontainer (branch) does specify a version but in our local environments, we might all have different versions of poetry with different behaviours. This is not good

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this long living thread about enforcing Poetry versions, still unsolved.
We could specify it in install_dependencies.sh, which is what we trigger when we do make setup-dev:
$ curl -sSL https://install.python-poetry.org | POETRY_VERSION=1.8.2 python3 -

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The recommended way of installing poetry these days is using pipx.

Copy link
Contributor

@ireneisdoomed ireneisdoomed left a comment

Choose a reason for hiding this comment

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

Really nice, thanks again!

@@ -99,3 +102,10 @@ repos:
rev: 0.4.1
hooks:
- id: pydoclint

- repo: https://github.com/python-poetry/poetry
rev: "1.8.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this long living thread about enforcing Poetry versions, still unsolved.
We could specify it in install_dependencies.sh, which is what we trigger when we do make setup-dev:
$ curl -sSL https://install.python-poetry.org | POETRY_VERSION=1.8.2 python3 -

.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@d0choa d0choa merged commit a3d7614 into dev Mar 7, 2024
4 checks passed
@d0choa d0choa deleted the do_precommit_poetry branch March 7, 2024 12:20
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.

2 participants