sadikkuzu commented 2 months ago

Description

Resolves #571 and #572

before:

after:

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.

adiroiban commented 2 months ago

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 :)

sadikkuzu commented 2 months ago

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.

sadikkuzu commented 2 months ago

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

sadikkuzu commented 2 months ago

@adiroiban hi, Can you approve remaining workflow runs?

adiroiban commented 2 months ago

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

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.

sadikkuzu commented 2 months ago

It seems ok now:

adiroiban commented 2 months ago

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.

sadikkuzu commented 2 months ago

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 👍🏼

adiroiban commented 2 months ago

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.

sadikkuzu commented 2 months ago

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

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.

adiroiban commented 2 months ago

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.

sadikkuzu commented 2 months ago

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

SmileyChris commented 1 month ago

Bleeding edge isn't the solution here.

adiroiban commented 1 month ago

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

twisted / towncrier

Add --bleeding-edge into pre-commit autoupdate #590

Description

Checklist