search

twisted / towncrier

Manage the release notes for your project.
https://towncrier.readthedocs.io
MIT License
755 stars 107 forks source link

pre-commit autoupdate seems to fail #571

Closed HealthyPear closed 1 month ago

HealthyPear commented 6 months ago

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 commented 6 months ago

Thanks for the report

I see that pre-commit support was added here https://github.com/twisted/towncrier/pull/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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

symonk commented 3 months ago

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 commented 3 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.