kiesraad / abacus

Abacus, software voor verkiezingsuitslagen en zetelverdeling
https://kiesraad-abacus.pages.dev
European Union Public License 1.2
19 stars 6 forks source link

Fix CLA Bot so that it checks against the contributors file on our main branch #513

Open jschuurk-kr opened 3 weeks ago

jschuurk-kr commented 3 weeks ago

It seems that if you fork the repo, submit a PR and put your account name in contributors.yml in the main branch of your fork, the CLA Bot thinks you have agreed to the CLA. The CLA Bot should check against the file in the main branch of our repo.

Example: https://github.com/kiesraad/abacus/pull/507

To do:

jschuurk-kr commented 3 weeks ago

test PR from my personal GitHub account: https://github.com/kiesraad/abacus/pull/514

jschuurk-kr commented 3 weeks ago

For the proper fix to cla-bot, the following options need to be investigated if they are valid replacements for the hard-coded values:

If neither of those options work, we can use getInput(). This basically moves the hard-coding from the cla-bot to the repo using cla-bot.


(update 30 Oct) Replacing "head" with "base" in context.payload.pull_request did the trick.

Hilbrand commented 3 weeks ago

A common process for signing off is to use the git built in --signoff option where the contributor adds a signed-off line to each comment. See for example the linux kernel process: https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1 . There are also tools to check on this and flag the contribution if the sign off is missing, like: https://probot.github.io/apps/dco/ Was that process considered? (or didn't the lawyers like that)

Sometimes a small patch exemption is also used to lower the bar for contributions. See for example: https://www.openhab.org/docs/developer/contributing.html#small-patch-exception That would make contributions like in #507 easier to accept, instead of requiring people to sign a paper just to be able to change an invalid url. So that might be something to consider?

P.s. this isn't probably the right place to address these questions. If there is a better place let me know.

ring-ring-ring commented 3 weeks ago

Any place is a good place to talk :-) I agree a solution like yours offers a much better user experience. The origin of the CLA is the advice from the EU PL 1.2 people who - if we want to accept external contributions - suggest it as a way to prevent future copyright issues. Our friends over at the Ministry of Health use a bit different implementation but the idea is similar. The one thing outside legalese that this CLA stuff adds is the human contact. It is nice as it allows me to properly say 'thank you for contributing, this is really cool work'.

dekkers commented 2 weeks ago

As far as I can see this can be fixed by triggering the workflow on the pull_request_target event instead of the standard pull_request event. This will run on the base of the PR and not on the changes in the PR so it should pick the contributors file from the main branch.

Other than that, I would recommend to use https://cla-assistant.io/ for the CLA. In my opinion you want to make it as easy as possible to contribute something. The cla-assistant is a lot easier than having having to print out a document, sign it, scan it and e-mail it.

jschuurk-kr commented 2 weeks ago

As far as I can see this can be fixed by triggering the workflow on the pull_request_target event instead of the standard pull_request event. This will run on the base of the PR and not on the changes in the PR so it should pick the contributors file from the main branch.

Thank you for looking into this! The CLA workflow is triggered on the pull_request_target event. However, our cla-bot uses the GitHub API to retrieve the contributors file. And it was using the head of the PR to determine which repo to retrieve that file from.