towncrier icon indicating copy to clipboard operation
towncrier copied to clipboard

Add --bleeding-edge into pre-commit autoupdate

Open sadikkuzu opened this issue 3 months ago • 14 comments

Description

Resolves #571 and #572

before: image

after: image

Checklist

  • [x] Make sure changes are covered by existing or new tests.
  • [x] For at least one Python version, make sure local test run is green.
  • [ ] Create a file in src/towncrier/newsfragments/. Describe your change and include important information. Your change will be included in the public release notes.
  • [x] Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • [ ] Ensure docs/tutorial.rst is still up-to-date.
  • [ ] If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
  • [ ] If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

sadikkuzu avatar Apr 28 '24 13:04 sadikkuzu

Thanks for the PR.

This is a good start, but it looks like a brute force fix for our problem.

It looks like bleeding edge just uses the latest version of any dependency.

Do we know which dependency is causing our issue.

While "bleeding-edge" might fix the current issue... I think that in the future we might end up with other transient error.

Also, I think that it's important to include the pre-commit update as part of our CI, to make sure we will not regress on this. That is, include the changes from #572 to check that this fixes the issue :)

adiroiban avatar Apr 28 '24 16:04 adiroiban

pre-commit autoupdate follows these git commands: https://github.com/pre-commit/pre-commit/blob/main/pre_commit/commands/autoupdate.py#L42-L61

To demonstrate so, I ran such commands:

git clone https://github.com/twisted/towncrier.git
cd towncrier
git checkout --track origin/571-pre-commit-hooks
git fetch origin HEAD --tags
git describe FETCH_HEAD --tags --abbrev=0

Then I got: 22.8.0

When I go to towncrier 22.8.0 version, there is no .pre-commit-hooks.yaml there.

image

sadikkuzu avatar Apr 28 '24 17:04 sadikkuzu

On the other hand, 23.10.0 version has such warning message:

image

sadikkuzu avatar Apr 28 '24 17:04 sadikkuzu

@adiroiban hi, Can you approve remaining workflow runs? image

sadikkuzu avatar Apr 28 '24 18:04 sadikkuzu

When I go to towncrier 22.8.0 version, there is no .pre-commit-hooks.yaml there.

Ok 28.8.0 is an older version.

But https://github.com/twisted/towncrier/tree/23.11.0 has that file.


23.10.0 version has such warning message:

We remove the branch that is used to do the release... so that we don't have too many branches in the auto-complete list

This is why you have that warning. It should not matter.

adiroiban avatar Apr 28 '24 18:04 adiroiban

Can you approve remaining workflow runs?

I have approved the CI.

I only now saw that this PR is targeting the ohter 571 branch... sorry, I was not expecting this... and I was just doing all kind of experiments on my branch.

adiroiban avatar Apr 28 '24 18:04 adiroiban

It seems ok now: image

sadikkuzu avatar Apr 28 '24 18:04 sadikkuzu

Many thanks for the help here.

I think I got this.

So the issue is with git describe that things that 28.8.0 is our latest version

$ git describe trunk --tags
22.8.0-230-g7221d5d

We release towncrier from a branch/tag and not from trunk

We then squash the branch...and the tag commit is lost.

I think that the fix here, is to do a new towncrier release, in which we make sure we don't use a squash merge.

adiroiban avatar Apr 28 '24 18:04 adiroiban

Moreover, this pre-commit step is not necessary: image

Remaining next steps alone do right actions: https://github.com/twisted/towncrier/blob/571-pre-commit-hooks/.github/workflows/ci.yml#L219-L225 👍🏼

sadikkuzu avatar Apr 28 '24 18:04 sadikkuzu

Thanks for your help and feedback.

You help was critical in finding the source of this bug.

I don't think that we should merge this PR and use bleeding edge.

Instead, we should fix the release process to add a note that release PR should not be squashed...and then do a new release without a squash.

adiroiban avatar Apr 28 '24 18:04 adiroiban

Thanks for your help and feedback.

You help was critical in finding the source of this bug.

I don't think that we should merge this PR and use bleeding edge.

Instead, we should fix the release process to add a note that release PR should not be squashed...and then do a new release without a squash.

My pleasure 🙏🏼 ✨

Yes, we may close this PR. New release process will fix the issue. 👍🏼

sadikkuzu avatar Apr 28 '24 19:04 sadikkuzu

Moreover, this pre-commit step is not necessary: ...

Remaining next steps alone do right actions: https://github.com/twisted/towncrier/blob/571-pre-commit-hooks/.github/workflows/ci.yml#L219-L225 👍🏼

For this I will propose a new pull-request later on.

sadikkuzu avatar Apr 28 '24 19:04 sadikkuzu

The idea of the CI jobs was to do a full pre-commit cycle, similar to how it is used by developers, not only the auto-update step that is needed for this bug.

adiroiban avatar Apr 28 '24 20:04 adiroiban

The idea of the CI jobs was to do a full pre-commit cycle, similar to how it is used by developers, not only the auto-update step that is needed for this bug.

Then it should be pre-commit install, not pre-commit only.

https://github.com/twisted/towncrier/pull/592

sadikkuzu avatar Apr 29 '24 04:04 sadikkuzu

Bleeding edge isn't the solution here.

SmileyChris avatar May 21 '24 05:05 SmileyChris

Please add your feedback and suggestion to the #571 PR THanks

adiroiban avatar May 21 '24 07:05 adiroiban