search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.95k stars 5.16k forks source link

CI: add PR check for DCO signoff #6554

Open kdomanski opened 2 months ago

kdomanski commented 2 months ago

This workflow will test whether every commit in a PR contains the signoff. If not, the check will be marked as failed and a comment will be added, explaining to the author what is missing.

Note that this workflow triggers on 'pull_request_target', not 'pull_request', because only then Github Actions can write comments in the upstream repo from a fork.

kdomanski commented 2 months ago

Self-review checklist

1. Is the submission free of defects and is it ready to be widely deployed?

Yes, I tested the workflow in a forked repo: image

2. Does the submission provide a "high impact" benefit to real-world users performing real-world tasks?

I believe so, yes. I'm told that the maintainers spend a considerable effort reminding PR authors about the signoff. This PR will automate the effort and remove the burden from the maintainers.

3. Is the copyright of the submission clear, non-gratuitous, and compatible?

Yes.

4. Does the submission follow guidelines specified in the Klipper documentation?

Yes.

5. Is the Klipper documentation updated to reflect new changes?

No documentation update is necessary. The general requirement for DCO signoff is already documented in the contribution doc.

6. Are commits well formed, address a single topic per commit, and independent?

Yes.

KevinOConnor commented 2 months ago

Interesting. Thanks. It certainly would be nice to have an automated check for that. One thing I'm not sure of, though, is that a large number of PRs have the signed-off-by line in the PR comments. I've been accepting that, and just copying the signed-off-by line into the commit when I squash-and-merge it. Probably most PRs are that way. Thus I'm not sure if this change would add a lot of "noise" to the PR comments.

I guess I'd want to get feedback from the other Klipper PR reviewers.

-Kevin

nelsongraca commented 2 months ago

I think even if it adds some noise to PRs it's a valuable addition, will warn and guide anyone who forgets to sign off a commit or missed that from the dev docs. It will be one less thing for reviewers to worry about (if anyone checks than upon review) and one less thing to worry/do for the maintainer when merging, I believe anything that improves or simplifies a workflow is good

kdomanski commented 2 months ago

Hey Kevin, I'm pretty sure you can still merge a PR with the check "in the red" if you're the repo admin. I could also remove the last step of the workflow - then a comment is made but the check is still green.

All things considered, I believe shifting the burden of the reminder from you onto an automation is worth an additional comment or two.

a large number of PRs have the signed-off-by line in the PR comments. I've been accepting that, and just copying the signed-off-by line into the commit when I squash-and-merge it.

We could definitely alter the contents of the automated notice to fit the needs of this project, e.g. point out the possibility of signing off in a separate comment.

github-actions[bot] commented 1 month ago

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.