Update CLA checker workflow - #44
Conversation
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
I think the logic around removing comments looks ok.
I'm not sure the check of cla removed is correct though. This will always fail when an author is relying on their name being on base and branching from before that. This will be both annoying for developers who do multiple PRs per release, and also make conflicts and duplications more likely.
I also think the grep should be more permissive in this workflow - we're checking for the author existing, not correct formatting of the file.
| if [ -f "CONTRIBUTORS" ]; then | ||
| if grep -q "^$AUTHOR" CONTRIBUTORS; then | ||
| if [ -f "CONTRIBUTORS.md" ]; then | ||
| if grep -q "| $AUTHOR |" CONTRIBUTORS.md || grep -q "| $AUTHOR " CONTRIBUTORS.md; then |
There was a problem hiding this comment.
Formatting this as a markdown table is sensible, but I think this check shouldn't be reliant on correct formatting, particularly with whitespace.
So keep as grep -q "$AUTHOR"
There was a problem hiding this comment.
I think we should keep the check for the username to be in between | and |. Perhaps we can relax with the spaces around the username. Imagine a substring collision in another column for a different entry...
There was a problem hiding this comment.
Yeah fair enough, something like grep -qE "|\s*$AUTHOR\s*|" CONTRIBUTORS.md (not tested!)
There was a problem hiding this comment.
That should work, I think we probably ned to escape the pipes?
grep -qE "\|\s*$AUTHOR\s*\|" CONTRIBUTORS.md
| echo "signed=true" >> $GITHUB_OUTPUT | ||
| echo "✅ $AUTHOR has updated their CLA signature in CONTRIBUTORS file." | ||
| if [ -f "CONTRIBUTORS.md" ]; then | ||
| if grep -q "| $AUTHOR |" CONTRIBUTORS.md || grep -q "| $AUTHOR " CONTRIBUTORS.md; then |
There was a problem hiding this comment.
Same as above
| const author = context.payload.pull_request.user.login; | ||
|
|
||
| // Check if contributor was on base but removed themselves from PR | ||
| const removedFromPr = signedOnBase && !signedOnPr; |
There was a problem hiding this comment.
Is this complex enough? Once the first PR is on they'll have signed on base. But I would imagine they don't need to add themselves to the CONTRIBUTORS.md in subsequent PRs as it's already on base. This logic will fail in that scenario. I think it should be checking the diff of the CONTRIBUTORS file and looking for their username being removed.
There was a problem hiding this comment.
hmmm.. my brain was frozen at 1am. Agree, we should test different scenarios.
There was a problem hiding this comment.
Good point and as discussed I've updated the workflow to handle the scenario where a contributor has already signed the CLA in the base branch. Here's what changed:
- a new step to check if CONTRIBUTORS.md was modified in the PR by comparing changed files between base and PR branch.
removedFromPrnow only triggers if the contributor was on base, not on PR, and they modified the CONTRIBUTORS.md file (intentionally removed themselves)cla_metnow passes if either contributor is signed on both base and PR branches, OR
contributor is signed on base branch AND didn't touch the CONTRIBUTORS.md file in their PR.
|
Tagging Jenny Hickson (@jennyhickson) in this in case she has thoughts on the removal process |
James Bruten (james-bruten-mo)
left a comment
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Thanks for adding the git diff logic, that looks like it should do what we want it to!
2 participants
Adding some changes to existing cla-checker workflow: