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

Automate the commit checks and move the checklist to CONTRIBUTING.md #110

Merged

Conversation

primeos-work
Copy link
Member

We can avoid the dependency on 13rac1/block-fixup-merge-action because there is a community-contributed gitlint rule for it 0. The contrib-disallow-cleanup-commits rule enforces that the subject/title does not contain fixup!, squash!, or amend!. We can also use the contrib-body-requires-signed-off-by rule to check that the body contains the Signed-off-by line.

We decided to move the checklist from the PR template to CONTRIBUTING.md so that contributors don't have to manually tick all checkbox entries for every PR.
The CI workflow already runs cargo check,
cargo test --all --all-features, and
cargo clippy --all --all-targets --all-features -- -D warnings.

The github.event_name == 'pull_request' check isn't required because of the on: [pull_request] statement at the beginning of the file.

Signed-off-by: Michael Weiss [email protected]


Tested via my fork:

We can avoid the dependency on `13rac1/block-fixup-merge-action` because
there is a community-contributed `gitlint` rule for it [0].
The `contrib-disallow-cleanup-commits` rule enforces that the
subject/title does not contain `fixup!`, `squash!`, or `amend!`.
We can also use the `contrib-body-requires-signed-off-by` rule to check
that the body contains the `Signed-off-by` line.

We decided to move the checklist from the PR template to CONTRIBUTING.md
so that contributors don't have to manually tick all checkbox entries
for every PR.
The CI workflow already runs `cargo check`,
`cargo test --all --all-features`, and
`cargo clippy --all --all-targets --all-features -- -D warnings`.

The `github.event_name == 'pull_request'` check isn't required because
of the `on: [pull_request]` statement at the beginning of the file.

[0]: https://jorisroovers.com/gitlint/contrib_rules/

Signed-off-by: Michael Weiss <[email protected]>
@primeos-work primeos-work mentioned this pull request Dec 13, 2022
11 tasks
Copy link
Contributor

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Nice to use gitlint, especially since I contributed that check to gitlint 😆

Comment on lines +19 to +24
gitlint \
-c general.ignore-fixup-commits=false \
-c general.ignore-fixup-amend-commits=false \
-c general.ignore-squash-commits=false \
--contrib contrib-body-requires-signed-off-by,contrib-disallow-cleanup-commits \
--commits "$(git merge-base origin/master HEAD)..HEAD"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather put a .gitlint file in the repository than using CLI flags, so contributors can run gitlint locally!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice to use gitlint, especially since I contributed that check to gitlint

Haha, nice xD

I'd rather put a .gitlint file in the repository than using CLI flags, so contributors can run gitlint locally!

Yes, I'd also prefer that. If you don't mind, I'd do it in a separate PR though (together with some other gitlint changes/improvements that I didn't include here). This PR already got bigger than expected and I didn't want to squeeze everything into that large commit (and I was also too lazy to repeat all of my tests for it tbh).

Copy link
Contributor

Choose a reason for hiding this comment

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

Bigger than expected with one commit? Hahah you clearly haven't seen my 800+ commit PRs yet.

But sure, go ahead! I'm not the maintainer here, so I have nothing to say anyways! 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

@matthiasbeyer You know that's not true and your opinion matters here :)
@primeos-work Thanks for the PR
bors merge

@bors
Copy link
Contributor

bors bot commented Dec 14, 2022

Build succeeded:

@bors bors bot merged commit efd94e3 into science-computing:master Dec 14, 2022
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.

3 participants