Skip to content
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

Add github workflow that checks if a private email address was used to contribute to the repo and warn in this case #80514

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

asl
Copy link
Collaborator

@asl asl commented Feb 3, 2024

Following the Discourse discussion, warn in case of a private email address was used in a PR.

to contribute to the repo and warn in this case.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 3, 2024

@llvm/pr-subscribers-github-workflow

Author: Anton Korobeynikov (asl)

Changes

Following the Discourse discussion, warn in case of a private email address was used in a PR.


Full diff: https://github.com/llvm/llvm-project/pull/80514.diff

1 Files Affected:

  • (added) .github/workflows/email-check.yaml (+36)
diff --git a/.github/workflows/email-check.yaml b/.github/workflows/email-check.yaml
new file mode 100644
index 0000000000000..4b729ddfa42d5
--- /dev/null
+++ b/.github/workflows/email-check.yaml
@@ -0,0 +1,36 @@
+name: "Check for private emails used in PRs"
+on: pull_request_target
+permissions:
+  pull-requests: write
+
+jobs:
+  validate_email:
+    runs-on: ubuntu-latest
+    if: github.repository == 'llvm/llvm-project'
+    steps:
+      - name: Fetch LLVM sources
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+
+      - name: Extract author email
+        id: author
+        run: |
+          git log -1
+          echo "EMAIL=$(git show -s --format='%ae' HEAD~0)" >> $GITHUB_OUTPUT
+
+      - name: Validate author email
+        if: ${{ endsWith(steps.author.outputs.EMAIL, 'noreply.github.com')  }}
+        uses: actions/github-script@v6
+        env:
+          EMAIL: ${{ steps.author.outputs.EMAIL }}
+        with:
+          script: |
+            const { EMAIL } = process.env
+            await github.rest.issues.createComment({
+              issue_number: context.issue.number,
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              body: `⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
+              Please turn off [Keep my email addresses private](https://github.com/settings/emails) setting in your account.
+            `})

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Logic looks good, but not exactly sure of all the details on how Github hides emails. Post-commit analysis of the emails in the commit log to make sure this has the intended effect would definitely be good, but was probably planned anyways.

Thanks for working on this issue!

Copy link
Collaborator Author

asl commented Feb 3, 2024

I already checked this in our temp-issue-tester repo and it seems to be working. We might miss some cases, but we can examine the things afterwards to see what they are.

@asl asl merged commit 067882c into llvm:main Feb 5, 2024
4 checks passed
@asl asl deleted the private-email-check branch February 5, 2024 03:42
@asl
Copy link
Collaborator Author

asl commented Feb 5, 2024

@boomanaiden154 Confirmed works on LLVM repo as well: #80627 (comment)

@boomanaiden154
Copy link
Contributor

Awesome! Hoping there aren't any edge cases that we run into.

@tru
Copy link
Collaborator

tru commented Feb 5, 2024

I think we should tweak this action a bit. It probably shouldn't spam a comment on every change. We can just update or keep one comment at the time. Also I wonder if we should link or include some expanded info why we request people to change this setting and what it does for us. It's not clear for anyone who haven't been part of the conversations why we want it to be public.

@boomanaiden154
Copy link
Contributor

boomanaiden154 commented Feb 5, 2024

Yep. The only issue with that is that the comment might get buried (as I've seen happen with the code formatting action), but the job should also fail (which would need another patch) when it writes a comment to make that visible. I've put it on my backlog to get those two things done, but if someone else gets to it first, that would be good too.

@DavidSpickett
Copy link
Collaborator

#80648 is a temporary fix if that helps.

@DavidSpickett
Copy link
Collaborator

Also I wonder if we should link or include some expanded info why we request people to change this setting and what it does for us. It's not clear for anyone who haven't been part of the conversations why we want it to be public.

Definitely.

If I'm a drive by contributor, I would be more likely to expose my email address if I see there's legitimate reason to do so. If I had other reasons to remain anonymous, I may choose to abandon the PR, but at least I had all the information I need to make that decision (and hopefully not be resentful to llvm as a result).

- name: Fetch LLVM sources
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this getting the whole repo each time? I wonder if there are some "shallow checkout" options that would apply here.

Or we could get it from the PR event, maybe, but I guess you already looked for that and I don't want to derail the chosen solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is user.email in https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request. Would need testing to confirm, but I'd guess that checking for null or noreply@github... in the string would work.

And possibly this could all be done in the YAML. Which is not necessarily a good thing so you don't have to change what you've got.

Just know that this is (allegedly) an option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emails could come from different places, but we mostly bother about email of author in the commit (which has to set up manually from via git config if one uses git CLI and private email address, so must be done explicitly) and I thought this is the most bullet-proof way to do this.

@asl
Copy link
Collaborator Author

asl commented Feb 5, 2024

I've added link to Discourse in ee06678

agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
…o contribute to the repo and warn in this case (llvm#80514)

Following the Discourse discussion, warn in case of a private email address was used in a PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants