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

pre-commit autoupdate seems to fail #571

Closed
HealthyPear opened this issue Dec 20, 2023 · 14 comments · Fixed by #572
Closed

pre-commit autoupdate seems to fail #571

HealthyPear opened this issue Dec 20, 2023 · 14 comments · Fixed by #572

Comments

@HealthyPear
Copy link

HealthyPear commented Dec 20, 2023

When I launch pre-commit autoupdate I get this

$ pre-commit autoupdate
[https://github.com/pre-commit/pre-commit-hooks] already up to date!
[https://github.com/psf/black-pre-commit-mirror] already up to date!
[https://github.com/adamchainz/blacken-docs] already up to date!
[https://github.com/rstcheck/rstcheck] already up to date!
[https://github.com/charliermarsh/ruff-pre-commit] already up to date!
[https://github.com/nbQA-dev/nbQA] already up to date!
[https://github.com/twisted/towncrier]
=====> /var/folders/2z/142033n17rbfy969s6h4hymw0000gn/T/tmphs0r_674/.pre-commit-hooks.yaml is not a file

So all other git hooks get updated, but it crashes only when trying to update this project.

I also tried to manually update the config file with what I see from your docs,

  - repo: https://github.com/twisted/towncrier
    rev: 22.13.0
    hooks:
      - id: towncrier-check

but the result doesn't change.

Also pre-commit clean doesn't seem to help.

@adiroiban
Copy link
Member

Thanks for the report

I see that pre-commit support was added here #499

I am not very familiar with pre-commit internals.

Can you please provide a self contained example so that I can reproduce this error?

@HealthyPear
Copy link
Author

I think the minimal example is to use what it proposed in the docs, so a .pre-commit-config file with just

  - repo: https://github.com/twisted/towncrier
    rev: 22.13.0
    hooks:
      - id: towncrier-check

@HealthyPear
Copy link
Author

It is worth noting that this happened from one day to another; it is not the first time I use towncrier with pre-commit

@adiroiban
Copy link
Member

I was not able to reproduce see the change from #572 and the result https://results.pre-commit.ci/run/github/48647797/1703077437.QjeBDGruRIWq9RS5mZgEHg

@HealthyPear
Copy link
Author

I see that in there you test only for the direct installation, you are not using autoupdate - perhaps only that causes a problem?

I can install the git hook, what fails is the update

@adiroiban
Copy link
Member

Thanks for the info.
As commented. I'm not very familiar with pre-commit.
This is why I was asking for the steps to reproduce this... for dummies :)

I think I got this. I was able to reproduce the error and I have pushed the steps to the PR.

Not sure what is going in there.

I will not have much time to investigate this.

My main concern was why our automated tests pass, while people are getting errors.

I am happy to review a fix for this.

Regards

@adiroiban
Copy link
Member

I was able to reproduce the error here https://github.com/twisted/towncrier/actions/runs/7276149695/job/19825390531?pr=572#step:6:18

@symonk
Copy link

symonk commented Mar 9, 2024

Thanks for the report/PR already - also suffering:

[https://github.com/twisted/towncrier] 
=====> /tmp/tmp1orupgt7/.pre-commit-hooks.yaml is not a file

Only started since adding the towncrier-check hook, not sure if it's something to open in pre-commit itself, or this repository

@adiroiban
Copy link
Member

Only started since adding the towncrier-check hook, not sure if it's something to open in pre-commit itself, or this repository

We alrady have this issue #571 for looking into this error on towncrier side.
We also have a PR #572 that reproduces the error


If you have time, please consider opening an issue in pre-commit repo and linking to this issue or PR

Link to reproducing the error

https://github.com/twisted/towncrier/actions/runs/7276149695/job/19825355701?pr=572#step:6:1

@SmileyChris
Copy link
Contributor

Only the 22.8.0 tag is actually on the towncrier primary (trunk) branch, so it's the only one that is found by pre-commit.

I've updated the 23.11.0 tag to point to the actual merged 23.11.0 commit rather than the side-branch one that was rebased. This doesn't update the release date in github, but the date on the tag itself is obviously new.

Now this results in:

$ pre-commit autoupdate
[https://github.com/twisted/towncrier] already up to date!

And as long as in the future we actually tag the released branch rather than the pre-rebased one, it'll work fine.

@adiroiban
Copy link
Member

adiroiban commented May 21, 2024

I think that is best to reopen this and fix it only after we have updated the release documentation.

Right now we merge PR with squash.
We can continue to do that, but we should add an exception for the release PR.

The release PR should be merged witout squash

I am trying to fix this in #571

@SmileyChris if you have time, please take a look at that PR and see if the release process makes sense.

Thanks!

@adiroiban adiroiban reopened this May 21, 2024
@adiroiban
Copy link
Member

Another thing is that it's good to have pre-commit auto-update as part of our automated tests so that if anything breaks in the future, we can detect it a bit easier

@SmileyChris
Copy link
Contributor

Agreed that it would be good to have them as part of the automated tests.

The release PR can be merged with squash if the tag is applied to the squashed commit rather than on the branch. But yeah, probably fine just better to not squash in the first place :)

@adiroiban
Copy link
Member

We use GitHub Releases to do the release... so the tag is created by GitHub... on a commit from the release branch.

I think that we should just not use squash in the first place

But I don't think that we can enforce this using branch protection :(
And it might be easy to miss this and just merge with squash.

adiroiban added a commit that referenced this issue May 21, 2024
…t autoupdate. (#572)

* Use pre-commit dog food.

* Run pre-commit autoupdate.

* Update .github/workflows/ci.yml

* Better name. Also run auto-update.

* Add news fragment.

* Update black.

* Update pre-commit hooks.

* Use latets towncrier.

* Use latest pre-commit-hooks.

* Use immutable versions

* Add info about release process.

* Apply fix from Sadik.

* Update .github/workflows/ci.yml

Co-authored-by: Chris Beaven <[email protected]>

---------

Co-authored-by: Hynek Schlawack <[email protected]>
Co-authored-by: Chris Beaven <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants