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 pre commit #459

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Add pre commit #459

merged 6 commits into from
Oct 11, 2023

Conversation

mshriver
Copy link
Collaborator

Depends on #458

@mshriver mshriver marked this pull request as ready for review November 22, 2022 13:02
@laDok8
Copy link
Contributor

laDok8 commented Nov 22, 2022

what about adding GH release job, so all releases are synchronized and automated

@mshriver
Copy link
Collaborator Author

@laDok8 I think that's a reasonable request, to automate more of the release process for the repo, as currently its only based on tagging, and creating GH releases is optional and non-standard.

I'd ask that we do that separately from this PR though, via a new issue. If you're interested it would be a great scope for you to learn more about packaging/publishing and GHA.

@laDok8
Copy link
Contributor

laDok8 commented Nov 22, 2022

thanks, i would love to 👍

@@ -0,0 +1,40 @@
ci:
autofix_prs: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its potentially helpful to leave this, eases the life for drive by contributors that just messed up syntax

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it set to false as contributors might not realize that someone/something else pushed new commits to their branch. I would expect contributors to have pre-commit installed locally, therefore they should run the checks before pushing the commit - either as an actual git hook or manually by pre-commit run.

There is still the possibility to fix it by adding a pre-commit.ci autofix comment to the PR. See https://pre-commit.ci/#configuration-autofix_prs

.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@ogajduse
Copy link
Member

pre-commit.ci autofix

Copy link
Member

@ogajduse ogajduse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK on @mshriver's patch.
I have rebased his patch and ran pre-commit. I would like to get at least one more ACK before we merge it.

Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explicitly spelling out the project choice
it fits with the expectations

@ogajduse ogajduse merged commit 3c6561c into RedHatQE:master Oct 11, 2023
6 checks passed
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.

5 participants