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.

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

fix(build): add pre-commit as a dev dependency to simplify local dev and CI #438

Merged
merged 3 commits into from
Mar 4, 2024
Merged

fix(build): add pre-commit as a dev dependency to simplify local dev and CI #438

merged 3 commits into from
Mar 4, 2024

Conversation

max-pfeiffer
Copy link
Contributor

@max-pfeiffer max-pfeiffer commented Mar 2, 2024 •
edited
Loading

I could not install pre-commit handler because the package was missing in [tool.poetry.group.dev.dependencies].

I also updated the configuration for the pre-commit handler as black and ruff packages were outdated. Also removed deprecation for ruff v0.3.0 config in pyproject.toml.

@max-pfeiffer max-pfeiffer mentioned this pull request Mar 2, 2024
Merged
@max-pfeiffer
Added pre-commit as development dependency, updated pre-commit config
3b0f6dc 
pre-commit package was not present in development dependency,
so pre-commit handler could not be installed locally.
Also simplified CI for linting a bit.

Removed black and ruff from dev dependencies

After thinking about it again, just using pre-commit
handling in CI is a simpler approach also for maintenance.
@max-pfeiffer max-pfeiffer force-pushed the feature/pre-commit-handler-dependencies branch from d55cf92 to 3b0f6dc Compare March 2, 2024 17:38
@max-pfeiffer max-pfeiffer changed the title Updated pre-commit setup for local development, removed pre-commit usage from CI Updated pre-commit setup for local development, simplified CI job for linting Mar 2, 2024
@alexanderankin
Copy link
Member

i think #440 is preferable as it does not introduce any extraneous changes, @totallyzen feel free to pick one and merge.

additionally we have an incorrect ruff config it looks like, which is fixed here, which i would rather 1) fix in a separate PR 2) fix after some releases because it may change the linting which just introduces more work (im not a big linting believer but we should eventually fix our config i guess)

@totallyzen totallyzen changed the title Updated pre-commit setup for local development, simplified CI job for linting fix(build): add pre-commit as a dev dependency to simplify local dev and CI Mar 3, 2024
@totallyzen totallyzen changed the title fix(build): add pre-commit as a dev dependency to simplify local dev and CI fix(build): add pre-commit as a dev dependency to simplify local dev and CI Mar 3, 2024
@totallyzen
Copy link
Collaborator

@max-pfeiffer
There is an argument against installing pre-commit as a dev dependency.
It's because it's another package manager that then hooks into git. This way "who is on top?" becomes a problem via poetry-ception.

However!

  • I did some digging with pre-commit and based on their issues, they aren't against your approach
  • I'm willing to recognise my own biases in this and my workflow wasn't really affected
  • I did a test with your branch, it worked for me too, so despite the differences, we can argue that your setup is simpler.
  • Therefore I'm happy to proceed like this, with one exception that we make it more permissive than just the strict version you've installed it with -> ^3.6 (I see you renovate user 😉 we'll get there on this project too, just give it time)

@alexanderankin

additionally we have an incorrect ruff config it looks like

It's not an incorrect one, the api "broke" between between 0.2 and 0.3 (config was made more explicit and old style was deprecated) and Max updated the config to match the new recommendation.
It's a common notion that 0.X versions allow unstable API 🙏

I copy-pasted some old settings from another project when I set up ruff for this project so I see no reason why not jump to latest practices. It is also a cleaner definition in the pyoproject.toml

@totallyzen totallyzen merged commit 1223583 into testcontainers:main Mar 4, 2024
7 checks passed
@totallyzen totallyzen mentioned this pull request Mar 4, 2024
Closed
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
Merged
bearrito pushed a commit to bearrito/testcontainers-python that referenced this pull request Mar 5, 2024
@max-pfeiffer @totallyzen
fix(build): add pre-commit as a dev dependency to simplify local de…
7eb4e73 
…v and CI (testcontainers#438)

I could not install pre-commit handler because the package was missing
in [tool.poetry.group.dev.dependencies].

I also updated the configuration for the pre-commit handler as black and
ruff packages were outdated. Also removed deprecation for ruff v0.3.0
config in pyproject.toml.

---------

Co-authored-by: Balint Bartha <[email protected]>
totallyzen pushed a commit that referenced this pull request Mar 6, 2024
@github-actions
chore(main): release testcontainers 4.0.0 (#433)
ed3b9fa 
🤖 I have created a release *beep* *boop*
---


##
[4.0.0](testcontainers-v3.7.1...testcontainers-v4.0.0)
(2024-03-06)

### Release Notes

The breaking changes are the ones we were able to easily track. If you
spot any new issues between `3.7.1` and `4.0.0`, please do report it and
we'll do our best to fix everything. The release is now

Some kudos from @totallyzen to folks who helped a great deal in starting
things again:
- kudos to @alexanderankin for his contribution on #426 
- kudos to @jankatins for feedback on various PRs including 
- kudos to @max-pfeiffer and @bearrito for their contributions as well


### ⚠ BREAKING CHANGES

* **compose:** implement compose v2 with improved typing
([#426](#426))
* **core:** add support for `tc.host` and de-prioritise `docker:dind`
([#388](#388))

### Features

* **build:** use poetry and organise modules
([#408](#408))
([6c69583](6c69583))
* **compose:** allow running specific services in compose
([f61dcda](f61dcda))
* **compose:** implement compose v2 with improved typing
([#426](#426))
([5356caf](5356caf))
* **core:** add support for `tc.host` and de-prioritise `docker:dind`
([#388](#388))
([2db8e6d](2db8e6d))
* **redis:** support AsyncRedisContainer
([#442](#442))
([cc4cb37](cc4cb37))
* **release:** automate release via release-please
([#429](#429))
([30f859e](30f859e))


### Bug Fixes

* Added URLError to exceptions to wait for in elasticsearch
([0f9ad24](0f9ad24))
* **build:** add `pre-commit` as a dev dependency to simplify local dev
and CI
([#438](#438))
([1223583](1223583))
* **build:** early exit strategy for modules
([#437](#437))
([7358b49](7358b49))
* changed files breaks on main
([#422](#422))
([3271357](3271357))
* flaky garbage collection resulting in testing errors
([#423](#423))
([b535ea2](b535ea2))
* rabbitmq readiness probe
([#375](#375))
([71cb75b](71cb75b))
* **release:** prove that the release process updates the version
([#444](#444))
([87b5873](87b5873))
* test linting issue
([427c9b8](427c9b8))


### Documentation

* Sphinx - Add title to each doc page
([#443](#443))
([750e12a](750e12a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Mar 11, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderankin alexanderankin alexanderankin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@max-pfeiffer @alexanderankin @totallyzen