-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
github: Add GitHub action to greet first time contributors #56093
github: Add GitHub action to greet first time contributors #56093
Conversation
3b65904
to
1f1f4c1
Compare
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.
No objection to this in general, but would like a bit more community input before we merge this.
Thanks for the swift review @stephanosio! And +1 to gather community input (I actually considered opening an RFC but that felt overkill?) |
AFAICS, that action adds a comment when a user opens their first issue/PR, not when the first PR is merged. p.s. I suppose you could make it comment when the first PR is merged by restricting the |
Yup, there's a bug open actions/first-interaction#226
Ya I can try -- but my guess it will then not properly determine that the now-merged-PR is the first one from this contributor https://github.com/actions/first-interaction/blob/main/src/main.ts#L149..L151 |
OK so following @carlescufi's recommendation, I've forked the actions/first-interaction GH action to add support for "comment on first PR merged". In the meanwhile, for those interested, below is the wording I came up with. I've added the GH action to this test repo, feel free to try it out: https://github.com/kartben/delete-me4.
|
@kartben misleading: CI workflows for first-time contributors must be manually approved on every push |
+1, I also caught that, thanks! Will update the wording. |
Actually, I just remembered thinking twice while writing it -- it is in fact the correct wording :) As this is implemented as a Github action, the bot will in fact comment on the PR only when someone will have manually approved CI. If you think that can help people understand better how we work the message could actually be reworded something like "A project maintainer just triggered our CI pipeline to run on your contribution, ...." |
Unfortunately, I haven't heard back from the maintainers of the actions/first-interaction (see actions/first-interaction#226). Can someone please help me get this fork https://github.com/kartben/first-interaction/tree/v1.1.2-zephyr hosted under the zephyrproject-rtos organization? |
1f1f4c1
to
ec2a90a
Compare
01aaf25
to
ccac693
Compare
Ah, OK! That makes sense.
I think that would be an improvement. |
ccac693
to
151db78
Compare
+1. I've fixed the wording. |
Take the PR out of draft and add TSC label -- if nobody gets to it by the next TSC meeting, it can be handled then |
@stephanosio please have another look when you get a chance. Thanks! |
I guess my question to this is pretty brief: why? |
Mmmh I guess I could reply briefly too... why not? The idea is simply to encourage first time contributors (which we get on a daily basis) to get more involved and to really make the point that we do value their contributions. While it certainly won't be the silver bullet that will get people to automagically consider becoming long-time contributors, my hope is that it can contribute to getting some of them to come back. FWIW, out of the 500+ unique individuals who contributed to Zephyr over the last 6 months, roughly 50% contributed 2 commits or less (in fact, 33% contributed only 1). Even if we capture a small portion of these folks, I'll consider this to be a win, but YMMV. |
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.
I am very concerned about the isFirstPull
function implementation:
zephyrproject-rtos/action-first-interaction@5d55935#diff-7934bf411fea192ad8cd69e0a12911648a2842cb0f2409a8fb67b41b7069d757R153
function isFirstPull(client, sender, curPullNumber, closed, page = 1) {
var _a;
return __awaiter(this, void 0, void 0, function* () {
...
return yield isFirstPull(client, sender, curPullNumber, closed, page + 1);
});
}
Here you are effectively querying every pull request that has ever been created in the repository whenever a pull request is opened or closed, with each query API request returning 100 of them.
We currently have a total of 38670 pull requests in the Zephyr main repository, and that means 387 API requests per every PR open/close!
This is simply not going to scale and we will hit the API request limit very quickly.
I've seen numerous PRs that are a first time contribution for someone fixing a typo or fixing a very minor issue with the documentation - and that's the only PR they ever do, it's a wrong assumption to assume that every contributor is going to submit multiple PRs to the project, maybe they're correcting an issue they saw and other than that they're entirely happy with how the code is. From my perspective I don't see the value this adds, if someone has something to contribute, they can contribute it, if not, then there's nothing to submit. |
Not all of them though! Just a subset, which is confusing. |
@kartben I have no objections to this PR, thanks for the effort in forking the original action and modifying it to add the functionality we need. The comments from @stephanosio however need to be investigated. I wonder if there is an easier way to find out? For example using https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#get-a-repository to get the contributor list and then check if the user is on that list? |
I was just starting to think about it and yes, you're probably right :) I got misled by the way the original action was doing things, and the fact that it said that there's no API to filter PRs by creator (see https://github.com/actions/first-interaction/blob/main/src/main.ts#L126) so didn't look/think further. I will look at @carlescufi's suggestion but it also looks like I can probably get all I need with the /search API ( |
Agreed. The search-based solution sounds quite promising. The contributor list approach has the following issue;
|
I tend to fall in the same camp here. I find such automated messages to be annoying and generally a turn off to contributing further. I also don't see any harm in trying something new to encourage contributions. If it works, great. If it doesn't it can be iterated to a better solution or disabled later. Most of the regular contributors will never even see the message. |
And so much easier too! Action has been updated in this PR: zephyrproject-rtos/action-first-interaction#1 |
fb1da87
to
30c6808
Compare
Adds the configuration file for a GitHub action that will greet first-time contributor the first time they: - open an issue - open a PR - get a PR of theirs merged Fixes zephyrproject-rtos#56092. Signed-off-by: Benjamin Cabé <[email protected]>
30c6808
to
f95999a
Compare
I get your point but I'd argue that only using "has 0 comment" as the criteria to determine if someone has engaged is probably not the best approach in the first place as it can miss instances where the original author made several comments // see e.g the following issues, by only looking at the first two pages of opened issues |
Adds the configuration file for a GitHub action that will greet first-time contributor the first time they: