By “come across a bit too strongly” I meant that it is a bit too strong for the triagers to be able to trigger the message. The current functionality makes sense for core devs requesting changes, I fully agree with that.
That’s definitely understandable. So essentially the awaiting changes and awaiting merges labels should remain as they currently are, and stay off limits to anyone who isn’t a core dev. Thanks for the clarification.
With that in mind, I remain by my initial proposal then: for the awaiting core review label to be triggered by triager reviews, but not contributor reviews.
Not for the purpose of devaluing the reviews of contributors, but for increasing the meaning of the awaiting core review label. Currently, a contributor can simply approve a PR without a full review or any commentary. Restricting the usage of the label to triagers would significantly increase the likelihood that the PR is actually ready for core review and improve the positive effect it has on the workflow.
As a contributor who has reviewed a decent number of PRs in the last month, I don’t feel as if this change would at all devalue my feedback. Usually the value I perceive from PR reviews comes from the author and core devs showing appreciation (either with a thumbs up or directly thanking me), not the changing of a label.
Agreed, this could be considered as an additional feature. It might be best to make the decision after gathering feedback from triagers and core devs once the role is officially added.
As far devguide goes, I finished making changes to the PR involving the summary of the role. Currently waiting on that to be merged/further reviewed before working on the next component, which will probably be a description of the different GitHub labels so that triagers know when it’s appropriate to use each of them. I’ve been gathering additional information on GitHub label usage from the core devs since there’s not any existing documentation on them.
Do we currently have a rough idea of what labels the triagers will be able to manually add to PRs? The ones that immediately come to mind would be the skip labels (skip news and skip issue) and then the various type labels, such as type-documentation and type-bug. Are there any others that the triagers should be able to add?
I personally think any labels except the automerge lab should be fine for triagers to add. Even the need backports are fine too. Anyone else think differently?
Further clarification: labels added automatically by bots (CLA signed/not signed, awaiting …) should be left alone. But other labels (except automerge) labels should be good to use by triagers.
Based on my understanding of the devguide section, the backport labels cause miss-islington to attempt to perform the backport automatically after the PR is merged. Unless that functionality is changed, I think those labels should probably be reserved to the core devs.
Here’s the list of cpython labels, omitting awaiting, CLA, automerge, and backport:
From that list, the ones I’m uncertain about for triagers would be DO-NOT-MERGE, invalid, (due to them coming across strongly) and potentially type-security. The distinction between a PR being type-bugfix vs type-security might not be immediately clear. As an example, I initially had assumed that this PR would have been considered a security issue instead of a bugfix due to it being able to crash the interpreter with a segfault.
Also, it might be worth taking the time to make the labels across the Python repos as universal as possible. This might be going outside of the scope of the triager role specifically, but I figured it was relevant since we’re going to be adding a GitHub labels section as part of updating the devguide.
Some of the labels may only be applicable to specific repos, but there are a few which essentially have the same purpose, but have slightly different names. For example, the devguide has the bug and enhancement labels, which could probably be converted to type-bugfix and type-enhancement respectively. This may help avoid having to redundantly explain both of them and avoid any potential confusion. There’s other examples where the equivalent may not be as obvious, I just picked two easy ones for demonstration.
Remind me, does miss-islington simply create the backport PR or does it also merge the backport automatically?
In any case, I would expect triagers to have a good feeling about which PRs should be backported and which not. And keep in mind that a PR will always require a review from a core dev. I consider the labels as part of the PR that needs to be reviewed, so the core dev can always delete the labels in case of disagreement.
Thanks for the clarification. So there is indeed no real danger in adding the needs backport labels. I also agree with the rest of your post. In short: use common sense when triaging and don’t abuse your powers
Oh, good to know! I wasn’t aware that the awaiting merge label also had to also be present. In that case, there shouldn’t be an issue with triagers being able to add the backport labels since it requires a core dev to explicitly add awaiting merge, and then approve the PR. Would it be helpful to briefly mention this in the devguide? Currently, it is just phrased as:
After the pull request has been merged, miss-islington (bot) will first try to do the backport automatically.
This suggestion might be somewhat outside of the scope of initial addition of the team, but there was a potential permission that I thought about which would be highly useful for the triagers to be able to have (I’m not certain if this is already included in the triager permissons, let me know if it is).
Currently, when reviewing PRs on GitHub, the only way for contributors to add reviewers is through the use of something like /cc @coredev0 @coredev1 @coredev2, which essentially just sends a notification to the mentioned core dev, depending on their settings. However, I think it would be quite useful if triagers had permissions to directly add core developers to the reviewers on the PR.
Presumably, they would add anyone on the expert list who is in a category relevant to the PR. I’m working on updating that list to include GitHub usernames, but I’ve been prioritizing working on the GitHub label guide first.
Sometimes, the codeowners list helps with adding reviewers, but that generally does not stay in sync with the experts list, and there are a number of PRs that have no reviewers assigned to them. I think this would make the process of finding PRs that are relevant to their areas of expertise faster for the core devs, which could increase their volume of reviews within the same amount of time.
I think it would be better to get people to keep CODEOWNERS up-to-date and then see how to potentially use that file with the issue tracker when it moves to GitHub.
I definitely agree with keeping the codeowners list current, but I think there still are some PRs which would benefit from traigers being able to manually add reviewers. For example, when updating the documentation on a particular module, it might be beneficial to consult the experts of the category itself rather than just the documentation experts.
Adding them as a reviewer so every single time might be overkill (which would happen if they were added as codeowners to the rst file) for typos and phrasing improvements, in those cases the documentation experts could review the PR. But in situations where more involved changes are made such as new sections added or clarification changes, it would be beneficial to add the experts of the module to the reviewers list of the PR.
Personally, I don’t find updating the codeowners list and providing triagers the permissions to add reviewers to be mutually exclusive. The codeowners list can be used to cover a large number of PRs automatically, but many PRs could benefit from the ability to manually add experts when their feedback is needed. The core devs already have this ability, but I think it would help save them some time if the triagers were also able to do this.
They aren’t, but there’s also considering the amount of work to set something like this up and to maintain it. This all has cost in terms of time that we have to consider when proposing anything for our workflow.
If the codeowners list were to be liberal enough to include experts for a module anytime the relevant documentation file was modified in any way, manually adding reviewers probably wouldn’t be needed then. I had assumed that wouldn’t be the case, since it might push an excessive number of notifications to the core devs from trivial changes that the documentation experts could review.
The scenario I had pictured was a recent typo PR that spanned across 20 files throughout the repository. Depending on how the codeowners list was set up, it could potentially add 20+ core developers to the review list for a typo fixing PR. To prevent this, I figured a decent alternative would be to allow triagers to assign experts on more of a case-by-case basis for documentation changes rather than GitHub automatically adding all of the potentially relevant experts.
Since I was curious about how much time it would take it set this up, I did some looking into the various permission levels on GitHub. By default, anyone with “Triage” permissions has the ability to “assign all issues and pull requests”. When you assign someone to an open PR, I believe it adds them as a reviewer.
So unless we specifically prevent this behavior, anyone on the Python triage team should be able to assign reviewers to a PR.
