Skip to content

Fix CLA check for first-time contributors #12758

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

Merged
merged 1 commit into from
Jun 9, 2021
Merged

Conversation

griggt
Copy link
Contributor

@griggt griggt commented Jun 9, 2021

Now that Github Actions requires maintainer approval for workflow runs on PRs from first-time contributors, the CLA check is performed on the wrong user id (workflow approver rather than PR author).

A recent example of this is PR #12718, seen here: https://github.com/lampepfl/dotty/runs/2764331890?check_suite_focus=true#step:3:4

@griggt griggt requested a review from sjrd June 9, 2021 04:01
@sjrd
Copy link
Member

sjrd commented Jun 9, 2021

According to https://github.community/t/how-to-get-the-author-of-a-pr/135089, the event type will be different based on what triggers the issue, and hence the way to access the login is different. How can we be sure that this will work in (at least) the following cases:

  • Existing contributor opens pull request
  • New contributor opens pull request, gets approved by maintainer
  • Pull request author (force-)pushes a new commit
  • Someone else (force-)pushes a new commit
  • Maintainer relaunches a build manually for their own PR, or for someone else's PR

?

@griggt
Copy link
Contributor Author

griggt commented Jun 9, 2021

How can we be sure that this will work in (at least) the following cases:

* Existing contributor opens pull request

* New contributor opens pull request, gets approved by maintainer

* Pull request author (force-)pushes a new commit

* Someone _else_ (force-)pushes a new commit

* Maintainer relaunches a build manually for their own PR, or for someone else's PR

I informally tested most of these situations by inspection over here: https://github.com/griggt/wf-by-branch/actions
which for example demonstrated that ${{ github.event.sender.login }} is a poor choice because it fails in the fourth case.

I can clean that effort up if you like and ensure it covers the cases above.

I'd certainly welcome any suggestions on how to achieve proper test coverage here. Unfortunately the official documentation seems to be somewhat lacking.

@griggt
Copy link
Contributor Author

griggt commented Jun 9, 2021

the event type will be different based on what triggers the issue

Also of note, the only event type which triggers the cla workflow is pull_request:

https://github.com/lampepfl/dotty/blob/fdbb94f29b05fb712c04b981f462ae65b447fc39/.github/workflows/cla.yml#L1-L3

@sjrd
Copy link
Member

sjrd commented Jun 9, 2021

the event type will be different based on what triggers the issue

Also of note, the only event type which triggers the cla workflow is pull_request:

Right. Then perhaps github.event.pull_request.user.login is indeed always set up, and hopefully it contains the right thing. I guess we can try it out...

@sjrd sjrd merged commit 662060d into scala:master Jun 9, 2021
@griggt griggt deleted the fix-cla-check branch June 9, 2021 08:50
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.

2 participants