-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
doc: add config for welcome bot #21074
Conversation
.github/config.yml
Outdated
|
||
Please make sure to update documentation and tests when appropriate, and follow the [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines) | ||
|
||
We get a lot of issues on this repo, so please be patient and we will review this as soon as we can. |
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.
Technically, this is correct, but maybe use "a lot of pull requests" here?
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.
Good catch. Updated.
# Comment when a first-time contributor's PR is merged | ||
# https://github.com/behaviorbot/first-pr-merge | ||
firstPRMergeComment: > | ||
Congratulations on merging your first pull request! 👍 |
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.
Will this be triggered even if we don't merge the GitHub way?
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, this app purely uses GitHub Apps to listen on webhook events, like the PR being closed and then checks if the PR was merged and if it was that individual's first PR to be merged. So it depends on GitHub's merged
property to be true for the 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.
We often don't have that...
I am wondering if being welcomed by a bot actually makes the project more welcoming? It would be great if the bot actually does something that people do all the time: check if their commit messages match the guideline (instead of throwing the text at them), inform them about the 48/72 hour wait and the CI must be started by a collaborator (or even better, start a CI for them), give them a check list for the PR to get merged, .etc |
Is there evidence that this sort of thing is more helpful than the Github issue/PR templates? It seems like the templates cover (or at least could cover) at least this particular case. |
Heads up: if #21074 lands before this, it would be nice to mention that the |
@joyeecheung Which PR did you mean to link to? |
Probably #21059. |
I don't have empirical evidence of it, but can anecdotally say that many contributors ignore the templates, or don't read them entirely. Adding a post-facto comment is a second opportunity to clarify requirements and set expectations, and it reduces the likelihood that a collaborator will have to spend extra time explaining this stuff on new pull requests and issues.
Good call. Added.
Also added.
Checking that an issue or pull request adheres to the guidelines is indeed possible, but beyond the scope of what the welcome bot can do. We could of course write a custom bot for this, but the welcome bot can be a starting point. |
If we decide we want this, it might be better to add this little feature to https://github.com/nodejs/github-bot instead of introducing another bot. |
@mscdex I don't think it's necessary to make the github bot handle everything. We depend on it to communicate between GitHub and Jenkins so it's better to keep it dedicated to that particular job. Also we don't have to maintain yet a bunch of code and tests for welcoming new contributors if we just use an existing bot. |
@joyeecheung The nodejs/github-bot isn't strictly for Jenkins-related activities. It also tags PRs for example. I cannot imagine adding a 'welcome new contributor' message to the existing bot would be a maintenance burden, especially since that project already has any necessary Github-related testing functionality. |
☝️ I agree with this statement. This PR seems to have gone stale. Would love to get input from folks who were present when the idea was conceived: @bnb @hackygolucky @obensource @Tiriel |
|
||
# Comment when a first-time contributor's PR is merged | ||
# https://github.com/behaviorbot/first-pr-merge | ||
firstPRMergeComment: > |
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 think this can be removed since we rarely use the merge button and have our own workflow. It can be added when we adopt something else (e.g. a Jenkins job that merge the PR for us)
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.
Maybe we should leave it in for cases when the merge button is used?
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.
@zeke I am fine with leaving those texts. Although just for the sake of explanation, I am not actually aware of anybody using the button...at least not for first-time PRs. If you look at the PRs that are "merged", basically they are all opened and merged by collaborators, because right now if you don't use the merge button, you will need to push the HEAD that you are about to push to master to the same PR branch in order to make it purple. Unless you are the person that's going to push to master real soon, you are unlikely to get to the final state of that commit and push it to the PR branch right before it's pushed to master.
https://github.com/nodejs/node/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged
.github/config.yml
Outdated
|
||
For non-trivial changes, pull requests must be left open for *at least* 48 hours during the week, and 72 hours on a weekend. | ||
|
||
If this pull request changes includes code changes, the CI must be started by a collaborator. |
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.
Can you link collaborator
to https://github.com/nodejs/node/blob/master/GOVERNANCE.md#collaborators ?
.github/config.yml
Outdated
|
||
Please make sure to update documentation and tests when appropriate, and follow the [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines) | ||
|
||
For non-trivial changes, pull requests must be left open for *at least* 48 hours during the week, and 72 hours on a weekend. |
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.
Can you add (in UTC time) before being merged
at the end to avoid confusion?
newIssueWelcomeComment: > | ||
👋 Thanks for opening your first issue here! If you're reporting a bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can. | ||
|
||
To make it easier for us to investigate your issue, please follow the [contributing guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/issues.md). |
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.
It would be helpful to list common wrong repo
cases here (e.g. npm
, v8
, node-gyp
, .etc) but we can do that in a future PR.
@mscdex We can come back deleting this bot if someone actually takes the time to implement this in our own GitHub bot anyway. Otherwise this idea would just never happen. |
@joyeecheung thanks for the suggestions. I made some updates. |
There should be clarity on what this will accomplish because otherwise I'm going to -1 this PR. As is, I don't think a bot is particularly welcoming and IMO those type of things tend to be more annoying than helpful. A real human being taking the time to respond will always be infinitely more welcoming. Reading over the template, it's a weird mix of welcoming and rule reinforcement that I think will just get ignored, much like the issue template already is. What it's really adding is more spam for collaborators to deal with. |
Just checking in here again. There seems to be some contention about whether this change will be useful. What do folks think about giving this a try, iterating on the messaging as needed, and reserving the right to remove the bot if it does not prove to be helpful? |
What's the status on this one? |
@jasnell it's ready to go! Here's how I think we should proceed:
|
I'm still -1 on adding more bots that automatically post comments. FWIW I already don't care for the automated CI build bot messages because it shows up as a comment when looking at a list of PRs, even though it's not really a comment. It would be nice if Github at least had some way of being able to not include automated/bot comments in comment counts. |
I'm going to close this as not reaching consensus. However, if someone wants to advocate for it (perhaps @zeke?), I'd recommend putting it on the CommComm and TSC agendas. If both groups determine, effectively, "We hear the concerns, but think that trying this and iterating on it (or backing out if doesn't work out) is the way to go", then we can move forward. But I don't expect conversation here to resolve it. Anyway, if someone wants to put it on the agenda, re-open or leave a comment asking that someone else re-open it. |
Oh well. I thought this was worth a try, but not enough to campaign for it. 🤷♂️ |
This pull request adds configuration for https://github.com/behaviorbot/welcome, a bot that comments on issues and pull requests from first-time contributors. This was discussed during this week's Node.js Collaborator Summit as a way of making the Node.js project more welcoming to new contributors.
When and if this configuration lands in the repository, we'll need to actually install the bot. Maybe @Trott can help with that.
Special thanks to @hiimbex for creating this bot 🙏
// @bnb @hackygolucky @obensource @Tiriel