better integration with github features

[jyn514]

there are lots of features in triagebot that have substantial overlap with github features. off the top of my head:

• S-experimental is mostly the same as a draft pull request
• bors r+ is mostly the same as hitting "approve" on a PR
• r? name is mostly the same as assigning a reviewer, except triagebot uses "assignee" instead of "reviewer"
• ping groups are mostly the same as github teams
• assign.owners / mentions / ping have considerable overlap with github CODEOWNERS

can we unify some of these? my concern is not really the amount of code in triagebot, i don't care about that, my concern is that new contributors (and often even existing contributors!) do not know how to use triagebot and have to look it up. i would love to make it so that doing the "obvious" thing is the same as doing the right thing.

[Urgau]

• S-experimental is mostly the same as a draft pull request

Since #1968 we no longer consider

• bors r+ is mostly the same as hitting "approve" on a PR

bors != triagebot but even then, I you said it's "mostly" the same, idk what we could do for that one; most people know that an GitHub approval doesn't mean "try merging the pr"

• r? name is mostly the same as assigning a reviewer, except triagebot uses "assignee" instead of "reviewer"

What would you like to see unified here? triagebot already using GitHub's using the "Reviewers" section in GitHub, one of the reason we have r? is to allow contributor that are not member to request review from someone.

• ping groups are mostly the same as github teams

Same as above, teams can only be pinged if you have the permissions, which only members I think have. Maybe we can change that. We should ask T-infra about that.

• assign.owners / mentions / ping have considerable overlap with github CODEOWNERS

This one is interesting, our assign.owners is mostly out of date and CODEOWNERS could indeed solve that at a higher level. I see on the docs that we can make it not required.

I know T-infra is looking into CODEOWNERS it for rust-lang/team, we'll see how it goes for them.

[jyn514]

Since #1968 we no longer consider

this is not exactly the same; it just avoids assigning labels. i think draft prs should be treated exactly the same as experimental prs: we should not apply autolabels or autopings to either, and we should add S-experimental to anything that's a draft pr.

bors != triagebot but even then, I you said it's "mostly" the same, idk what we could do for that one; most people know that an GitHub approval doesn't mean "try merging the pr"

existing contributors know this. but new contributors do not. i have seen people close a PR that's in the queue because they saw bors r+ and assumed the PR had already been merged.

What would you like to see unified here? triagebot already using GitHub's using the "Reviewers" section in GitHub, one of the reason we have r? is to allow contributor that are not member to request review from someone.

no, it is using "assignees", which is not the same. i think if we switched to "reviewers" i would be happy here.

Same as above, teams can only be pinged if you have the permissions, which only members I think have. Maybe we can change that. We should T-infra about that.

seems reasonable. i don't feel strongly about this one, except that i think it is confusing to have adhoc-groups that do not have a corresponding github team. maybe CODEOWNERS can help here.

[apiraino]

this is not exactly the same; it just avoids assigning labels. i think draft prs should be treated exactly the same as experimental prs: we should not apply autolabels or autopings to either, and we should add S-experimental to anything that's a draft pr.

Do "draft" pull requests benefit of all the CI/check tooling of a S-experimental pull request?

we should add S-experimental to anything that's a draft pr

I might be wrong but I've always seen S-exp like "playgrounds" for people to work and iterate on maybe using the CI to test the changes. Drafts are for me mostly incomplete things (well, drafts).

[Kobzol]

I remember this being discussed on Zulip a few years ago, but I can't find it, of course 😆 I'm generally in favor of moving our processes to be as "GitHub UI-native" as they can be, to be familiar to contributors coming from different projects hosted on GitHub.

I think that draft PRs are a great example of this, and we already did some steps recently to make them "more native", for example draft PR will not automatically assign a reviewer now, nor set the S-waiting-on-review label. It is currently still labeled with other labels, although I'm not sure if that's a problem.

Do "draft" pull requests benefit of all the CI/check tooling of a S-experimental pull request?

Yes, draft PRs should be exactly the same as open PRs in terms of CI jobs.

One problem with draft PRs is that they might not be granular enough. We have various states of "not being ready", that could be "experimental work, do not even look" or "blocked on some other PR" or "WIP, do not review yet", etc. That's hard to convey with just a draft status, especially since different people use that feature differently. Using labels allows us to distinguish these cases.

bors r+ is mostly the same as hitting "approve" on a PR

While I agree that it would be nice, there are some issues with this. Not everything can be expressed with the approval button (rollup, priority, approving on behalf of someone else), and also we can't really sync the bors permission list with who can approve PRs. I think that all teams with write permissions on r-l/r can approve PRs (in the sense that their approval will have the green color in the UI), but that's not the same set of people who have r+ rights on the repository. It would be nice if bors could at least mark PRs as being approved (in the GH UI's sense of the word) by the reviewer after they post @bors r+, but IIRC this is not technically possible.

no, it is using "assignees", which is not the same. i think if we switched to "reviewers" i would be happy here.

This is a bit weird distinction done by GH, and I think it is mostly arbitrary that triagebot sets assignees. I guess the idea is that the person will be "responsible" for reviewing and merging the PR. I think that we could change this rather easily, we would just have to let people know so that they can modify their scripts or GH queries.

assign.owners / mentions / ping have considerable overlap with github CODEOWNERS

Sure, but also they seem to be more granular. With CODEOWNERS GH would I think forcefully ping and request a review from the person owning the file/directory, which is IMO too noisy/spammy.

I agree that adhoc-groups are weird though. With the new triagebot assignment logic, I would ideally like to remove adhoc groups, and just have teams be responsible for review, with people setting them on vacation or setting review capacity limits to go on or off the review queue.

[xizheyin]

Is it possible to send rustc-dev-guide to first time contributors, e.g. https://rustc-dev-guide.rust-lang.org/contributing.html? Learning to read basic documentation is essential for contributing to a project, but sometimes they don't know which page to look at as a first step. We could post something in PR comments for newcomers that is at least helpful for PR submissions.

[Kobzol]

We do actually have code in triagebot for this already (

triagebot/src/handlers/assign.rs

Line 200 in 1c72dfc

let welcome = if false

), but it was deactivated because it was too expensive to check if someone is a first-time contributor.

@ehuss I wonder if we could make that check be much "dumber", by just checking the GitHub author association? https://docs.github.com/en/graphql/reference/enums#commentauthorassociation

[xizheyin]

Intuitively, we can go through all the submitted records at one time, and then record the github id or email address locally (According to thanks.rust-lang.org, there are about 10000 contributors to the main branch), so as to avoid querying all the records through github api every time.

[ehuss]

by just checking the GitHub author association?

Seems like a potential route to try. Whatever technique is used also needs to be mindful if it works on forks (EDIT: though that might be a theoretical problem, we may not have any forks). I have a memory that this also gives false positives, but I don't remember now why that might be.

we can go through all the submitted records at one time

That sounds like it would be expensive, and difficult to keep it in sync. I'd want to be cautious of adding a lot of complexity for something that is mostly a minor feature.

[Kobzol]

Another simple solution is to simply remember users in a triagebot table. If we see the user for the first time at a repo, send the comment..

[xizheyin]

Another simple solution is to simply remember users in a triagebot table.

Yes, that's what I mean. Every time pr is opened, you only need to look up the table locally, and you don't need to query through github.