[CI] Make email check workflow fail when author's email is private in Github UI #148694
Conversation
|
@llvm/pr-subscribers-github-workflow Author: Udit Kumar Agarwal (uditagarwal97) ChangesProblem Solution Full diff: https://github.com/llvm/llvm-project/pull/148694.diff 1 Files Affected:
diff --git a/.github/workflows/email-check.yaml b/.github/workflows/email-check.yaml
index 904ad718f97dd..35cbcd3c810eb 100644
--- a/.github/workflows/email-check.yaml
+++ b/.github/workflows/email-check.yaml
@@ -26,8 +26,11 @@ jobs:
# Create empty comment file
echo "[]" > comments
+ # If author's email is hidden in GH's settings, github.event.pull_request.user.email
+ # will be null and PR will be authored by noreply.github.com.
- name: Validate author email
- if: ${{ endsWith(steps.author.outputs.EMAIL, 'noreply.github.com') }}
+ if: endsWith(steps.author.outputs.EMAIL, 'noreply.github.com') ||
+ github.event.pull_request.user.email == ''
env:
COMMENT: >-
⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.<br/>
@@ -39,6 +42,9 @@ jobs:
[{"body" : "$COMMENT"}]
EOF
+ # Fail this job.
+ false
+
- uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 #v4.3.0
if: always()
with:
|
|
|
|
Here's an example of a PR (at llvm/llvm-project) where this workflow is passing but the PR was merged with |
|
|
5 similar comments
|
|
|
|
|
|
|
|
|
|
Signed-off-by: Agarwal, Udit <[email protected]>
uditagarwal97
force-pushed
the
private/udit/email_check
branch
from
daf6fc6 to
bbde668
Compare
|
|
|
|
|
PR is ready for review. Based on contribution history, tagging @asl @DavidSpickett @vbvictor for feedback. |
|
This looks reasonable to me @tstellar @DavidSpickett @boomanaiden154 any objections? |
There was a problem hiding this comment.
My understanding of this is that before we looked at the commit and if that had a valid email address, that's enough.
The problem is that the email address used when merging is actually governed by GitHub settings not what's stored in git.
So this changes to GraphQL to do the check, but for the PR author, the way to make the email public is the same as before.
Correct?
(I ask you to confirm because at first glance the description feels like 2 layers of settings but in fact, one of them, the actual git commit, is ignored by GitHub)
There was a problem hiding this comment.
Is there documentation on what the GraphQL query is supposed to return given the different settings?
I know Github lets me select from multiple emails when merging. I'm not sure if there's a default or how it gets setup and I would be more comfortable approving this patch with a better understanding of how these all interact.
@boomanaiden154 Workflow run after I changed my default public email: https://github.com/llvm/llvm-project/actions/runs/16300461944/job/46033416527?pr=148694#step:3:24 (compare it with the previous run https://github.com/llvm/llvm-project/actions/runs/16279496749/job/45966062207) Regarding official documentation, the closest one I found is: https://docs.github.com/en/graphql/reference/objects#user |
There was a problem hiding this comment.
Workflow run after I changed my default public email: https://github.com/llvm/llvm-project/actions/runs/16300461944/job/46033416527?pr=148694#step:3:24 (compare it with the previous run https://github.com/llvm/llvm-project/actions/runs/16279496749/job/45966062207)
Changing my default public email also changes the email returned by GraphQL.
Regarding official documentation, the closest one I found is: https://docs.github.com/en/graphql/reference/objects#user
user.email returns the user's publicly visible profile email.
That looks good to me. I can't imagine anything other than the default email would be used when merging a PR for someone.
|
I think @boomanaiden154 has the technical aspects covered, they can be the approver. The new description is much clearer, thankyou!
This might explain why I've seen a number of noreply addresses slip through. |
Orthogonal to this PR, why don't we cause the workflow to fail when user's email is private? That way, the gatekeepers will be aware of the use of private emails? IMO, just adding a comment is not sufficient and it might get missed by the gatekeepers/reviewers say, when the PR received many comments. |
Not entirely sure. I think I remember there being some discussion about that on the original PR/some follow up commits. I agree that it would probably be good to make the workflow fail rather than just leaving a comment. That should probably be done as part of a separate patch though. |
I'll create a separate PR to make the workflow fail |
…email is private in Github UI" (#149186) Reverts llvm/llvm-project#148694 The workflow is failing if user's email is not listed publicly on your GH profile. This is different from not having your email public on Github (in Github email settings page vs. email field in Github profile/email settings).
Problem
Currently, the email check workflow uses
gitto see email used for the last commit but the email address used when merging is actually governed by GitHub settings not what's stored ingit. Due to this, the email check workflow passes even when the author's email is private in Github.We saw several such cases in our fork of llvm. See intel/llvm#17675
Solution
Try to find user's email using GH's GraphQL APIs. User's email will be null if it's hidden in the profile.