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

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

Closed

Conversation

sadikkuzu
Copy link

@sadikkuzu sadikkuzu commented Apr 28, 2024

Description

Resolves #571 and #572

before:
image

after:
image

Checklist

  • Make sure changes are covered by existing or new tests.
  • 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.
  • 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.

Signed-off-by: SADIK KUZU <sadikkuzu@hotmail.com>
@sadikkuzu sadikkuzu requested a review from a team as a code owner April 28, 2024 13:49
@adiroiban
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

image

@sadikkuzu
Copy link
Author

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

@adiroiban
Copy link
Member

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
Copy link
Member

adiroiban commented Apr 28, 2024

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
Copy link
Author

It seems ok now:
image

@adiroiban
Copy link
Member

adiroiban commented Apr 28, 2024

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
Copy link
Author

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

@adiroiban
Copy link
Member

adiroiban commented Apr 28, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

sadikkuzu commented Apr 29, 2024

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.

#592

@SmileyChris
Copy link
Contributor

Bleeding edge isn't the solution here.

@adiroiban
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants