-
Notifications
You must be signed in to change notification settings - Fork 132
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 a reminder when contributor is new to ping all contributor bot #2484
Merged
yucheng11122017
merged 47 commits into
MarkBind:master
from
KevinEyo1:2457-ping-new-contrib
Apr 11, 2024
Merged
Changes from 15 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
5444909
a
KevinEyo1 a4a4d72
Revert test changes
KevinEyo1 ea3aca7
Merge branch 'MarkBind:master' into master
KevinEyo1 c0f79a2
Merge branch 'MarkBind:master' into master
KevinEyo1 45b8846
Merge branch 'MarkBind:master' into master
KevinEyo1 21628f7
Merge branch 'MarkBind:master' into master
KevinEyo1 969865f
Add how to check if permissions are given
yucheng11122017 46397e7
Merge branch 'master' of https://github.com/MarkBind/markbind
yucheng11122017 21fa90c
Merge branch 'master' of https://github.com/KevinEyo1/markbind
KevinEyo1 0bc66f7
Merge branch 'master' of https://github.com/MarkBind/markbind
yucheng11122017 0ebb7cd
Merge branch 'MarkBind:master' into master
KevinEyo1 f798b1d
Merge branch 'master' of https://github.com/MarkBind/markbind
KevinEyo1 48399bd
Merge branch 'master' of https://github.com/MarkBind/markbind
KevinEyo1 cb84513
Fix merge conflict
yucheng11122017 701183b
Merge branch 'master' of https://github.com/MarkBind/markbind
KevinEyo1 4a83981
Merge branch 'master' of https://github.com/MarkBind/markbind
KevinEyo1 85d3b9f
Add workflow
KevinEyo1 7e7cfb7
Merge branch 'master' of https://github.com/MarkBind/markbind
KevinEyo1 e5619fa
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 2c9e6b4
Fix workflow
KevinEyo1 de03628
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 15a926b
Update workflow
KevinEyo1 9a08f81
Merge branch '2457-ping-new-contrib' of https://github.com/KevinEyo1/…
KevinEyo1 79a6363
Update action
KevinEyo1 2b43560
Add echo
KevinEyo1 64c280d
Add log
KevinEyo1 01a0637
Update workflow
KevinEyo1 c4e8b52
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 e2f6301
Allocate space for scrollbar in nav components
jingting1412 eb69423
Fix merge conflict
yucheng11122017 c9c05c2
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 936dadc
Fix merge conflict
yucheng11122017 30d31f7
Test time
KevinEyo1 6056ba5
Merge branch '2457-ping-new-contrib' of https://github.com/KevinEyo1/…
KevinEyo1 88405dd
Remove logs
KevinEyo1 cfeffed
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 5b9e7d7
Revert "Fix merge conflict"
yucheng11122017 c092296
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 30996b3
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 d6324a3
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 7b5d6a9
Update files
KevinEyo1 b695c9b
Improve security and upadte message
KevinEyo1 6db70bc
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 8832b1a
Merge branch 'master' into 2457-ping-new-contrib
KevinEyo1 7c00b6e
Revert to version number
KevinEyo1 7e26f6f
Rename variable
KevinEyo1 03bde6f
Merge branch 'master' into 2457-ping-new-contrib
yucheng11122017 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
name: New Contributor Action | ||
on: | ||
pull_request_target: | ||
types: | ||
- closed | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} | ||
cancel-in-progress: true | ||
|
||
jobs: | ||
ping-new-contributor: | ||
if: ${{ github.event.pull_request.merged }} | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/github-script@v7 | ||
with: | ||
script: | | ||
const creator = context.payload.sender.login | ||
const opts = github.rest.issues.listForRepo.endpoint.merge({ | ||
...context.issue, | ||
creator, | ||
state: 'all' | ||
}) | ||
const issues = await github.paginate(opts) | ||
|
||
for (const issue of issues) { | ||
if (issue.number === context.issue.number) { | ||
continue | ||
} | ||
|
||
if (issue.pull_request) { | ||
return // Creator is already a contributor. | ||
} | ||
} | ||
|
||
await github.rest.issues.createComment({ | ||
issue_number: context.issue.number, | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
body: `**Welcome**, new contributor! | ||
|
||
Do remember to request someone to add you as a official contributor to our repository, if you want to of course! See the full list of contributors [here](https://markbind.org/about.html) ✨` | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part lgtm but I am thinking of an alternative
Instead of checking all prs and see if there is any previous prs by the pr creator, I am thinking if we could utilise Github's built-in function to get the current list of contributors and check if the person is inside
https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#list-repository-contributors
Not sure if this approach would speed up the process since we do not have to go through every closed pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LamJiuFong, thanks for the suggestion, I think it will be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @KevinEyo1 ! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LamJiuFong after testing, I realised that since we are running the job on merge, the author is already part of the list by the time the action is run, so it fails to detect the author as a new contributor. I will revert to the original implementation, and I will try to figure out if there can be performance enhancements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, sorry for missing that out
What about checking if the author has only one pr merged (which is the newly merged one), would that be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LamJiuFong I am trying to think of a more efficient solution as well but I think to check if the author has only one PR merged we would have to fetch all the PRs anyway (which is what is done in the current implementation) leading to a similar time taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke to Kevin about this previously and he did this check: Test run to check time which wasn't very long. So I don't think there is a need to overoptimize in this case.