Repository navigation
Adjusting the "Triager" role's permissions #460
Description
Activity
Here's the list of "35 fine grained permissions", but I don't see any for the things suggested.
Well this is rather disappointing then -- reading the list it does seem a fairly mixed bag. I can't see a feedback link on the original blog post or else I'd add a note somewhere.
A
Yeah, I looked carefully thought the list and compared it with what permissions Triagers already have, and I didn't see anything there that would actually be useful or appropriate for Triagers, at least that isn't already enabled.
For example, in addition to the items @AA-Turner mentioned above, marking PRs as closing issues, or vice versa (I can close them manually and it seems like a very triage-related thing to do, so its omission is rather inexplicable). Or, one thing that could be considered is being able to edit others comments like users with Write and above can, i.e. to fix formatting/markdown issues (a particularly common and useful task when I was the primary triager of issues on the Spyder repo).
It seems pretty useless for open source projects, as opposed to specific cooperate/enterprise needs (billing managers, etc).
Reacted by Adam Turner and Ezio MelottiAnother permissions that would be useful for triagers is the ability to add issues to projects, especially if we start using them instead of the
expert-*labels.It would, but unfortunately this permission doesn't seem to be available either.
Thanks, I gave them upvotes just in case!
Reacted by Adam Turner, C.A.M. Gerlach and Ezio MelottiI believe I might have found a way around the issue of triagers missing a number of useful, non-intrusive permissions that have been repeatedly requested here, on Discourse, Discord and elsewhere.
If you enable Restrict who can push to matching branches and Restrict pushes that create matching branches in the branch protection rules for all branches in the CPython repo (whatever branches currently have rules, plus
*to cover any other branch), and add the teams that currently have write access (so there's no difference for anyone who currently has access), this allows you to give the Triage team the "Write" role, which would allow them access to the various GitHub UI capabilities that core devs and triagers have been asking for them to have, without giving them any ability to actually create, push to or delete any branch, or merge PRs.The main issue is whether there are any other permissions that people feel are too sensitive to give to triagers that don't involve modifying the actual repository content.
The permissions this would potentially change are listed here. Looking carefully, its a lot more than I initially thought, and we'd want to ensure we're happy with it before making any such change, of course.
If not, one alternative could be an intermediate role, a "Trusted Triager", that has these permissions but not the much more critical ability to be able to push commits, merge PRs or otherwise affect the content of the repo itself.
These are the permissions that would be added, minus those that branch protect would prevent and that aren't applicable to the CPython repo (e.g. those that only apply to private repos or with features not enabled there), with commentary on them
- Create, edit, delete labels
Could be potentially useful in some situations/workflows, but perhaps not ideal for triagers to be able to delete labels (however, that would be a pretty egregious breach of trust). - Edit and delete comments on commits, pull requests, and issues
When triaging on other repos, I find this invaluable to quickly fix common formatting errors in user issues that can severely impact readability, allow copying and pasting source text and linking issues in OPs. Any abuse would be immediately obvious since this is all logged and visible on issues (posts sayedited by @usernameat the top and clicking brings up the full change history, which can be reverted, and any deleted comments leave a message stating so, who deleted it and the reason). - Hide comments on issues
This can be useful for dealing with spam, OT comments (including my own) and those that are out of date and make an issue/PR hard to follow. The comments are simply collapsed and state the reason for hiding them (OT, spam, out of date, etc) and anyone can uncollapse them, so it doesn't seem like there's much risk with this. - Lock conversations
Probably not needed for a triager, but any abuse would be obvious and easily reversible. - Transfer issues between org repos
Likely useful for triagers in selected cases, and otherwise low potential for damage/abuse. - Allow being marked as a codeowner
Can be useful to allow auto-tagging someone on PRs without making them a core dev permissions (e.g. @hauntsaninja with Tomli, and would be really useful to us on the PEPs repo), but also completely optional; only has any effect if a core dev adds them toCODEOWNERS. - Mark PR draft or as ready for review
Potentially quite useful for triagers, and can't do any harm on its own (without merge rights). - Apply suggested changes to PRs
Probably not needed, but any abuse would be obvious and reversible (just drop the commits). - Run and re-run GitHub Actions workflows
Very useful and has been frequently requested. - Create and edit GitHub Releases
Initially this one worried me the most, but since the CPython repo doesn't use releases at all, the worst that could happen is a triager randomly adds some, which would be a pretty egregious breach of trust, would be instantly obvious (anyone watching the repo would be pinged immediately) and they could simply be deleted.
Reacted by Ezio Melotti, Hugo van Kemenade, Petr Viktorin, Jelle Zijlstra, C.A.M. Gerlach and Oleg Iarygin- Create, edit, delete labels
I'm not sure who can approve that change.
If the proposal is stuck, you can open a SC ticket with a summary (and links to any previous discussion), and the SC will (eventually) find the proper people to consult and delegate to.Thanks, will do. Though I and others have mentioned this a number of different places, IMO it is worth opening a dedicated Python-Dev discussion (at least on Discourse) to gather any additional general feedback or concerns before going ahead with a SC ticket. As such, I've gone ahead and opened one.
You should ensure that current CPython release managers are comfortable with this proposal as they are the people who typically manage branch protection rules for CPython. In case they haven't already seen this: @pablogsal @ambv @Yhg1s and @ned-deily.
As far as we're talking specifically about the python/cpython repo, I think the change proposed by Cam isn't controversial:
- make triagers writers to the repo, too;
- restrict them from actual pushes to protected branches by using "Restrict who can push to matching branches".
It will be a little annoyin* then to the release managers if they use branch locking to remove and re-add proper team lists. But I think it's worth it for the added capability for triagers.
time(triaging) >> time(locking_branches_during_release)
Just to make sure: CPython does use tag protection, right?
I don't think you can push anything but branches and tags to GitHub, but I wasn't able to verify that.It is not clear to me what 'the proposal' is. Giving triagers write permission appears to makes them committers, which is surely not what we want.
16 remaining items
Thanks again @ambv ! To publicly document my testing so far:
Full testing results
- ✔️ I have the expected added permissions listed in
cpython,devguideandcore-workflow - ✔️ I cannot merge pull requests on those repos
- ✔️ I cannot push commits to protected branches on those repos
- ✔️ I retain full retain
Maintain-level permissions/functionality inpeps - ❕ My approve/request changes reviews now show up in color (arguably not ideal), but the bot labels still indicate whether a core dev has approved
- ❕ It looks like this automatically sets Triagers to Watching the relevant repos if they haven't already set custom notification settings, so they should all check their settings to ensure it matches what they want
- ❌ I can push new branches to the upstream repos (somewhat easy to do by accident) and modify or even delete existing (non-protected) ones. This means Step 3 in the SC-approved specification still needs to be implemented
- ❌ I can create, modify and delete Git tags on the upstream repos except for CPython, so Step 4 in the approved specification needs to be implemented.
So, of the specified steps in python/steering-council#144:
- ✔️ I can verify step 1 and 5 work as intended
- ✅ I cannot test step 2 directly (though it appears to work on the
pepsrepo for us PEP editors, and no core devs have reported problems) - 🔲 Step 3, protecting creating/pushing to all (
*, non-release) branches, still needs to be implemented for those four repos - 🔲 Step 4,
*tag protection, still need to be implemented for the non-CPython repos
Specifically, to implement Step 3, you can add a new branch protection rule for
*matching all branches with theRestrict who can push to matching branches,Restrict pushes that create matching branches, EDIT:andEDIT 2: AndLock branchesandDo not allow bypassing the above settings, to avoid admins unknowingly bypassing this without creating a new branch protection rule, which would be required first in case of any deliberate new branch.Allow deletion, to allow users with theMaintainandAdminroles to delete branches they may accidentally create, as there in fact appears to currently be no way to avoid that.As discussed and decided on #475, the strong consensus was that core devs should normally avoid pushing own arbitrary working branches to the upstream repo, which is easy to do by accident (both locally and via the GitHub UI, as others have mentioned here), so ideally that rule should be left at the default of only org owners and admins (including RMs) being allowed to push new branches, which would prevent doing that by accident in the future.
The only potential issue is that it would prevent core devs from updating their existing branches, of which there are a very small number remaining. Here's the status of the remaining items, including a list of those branches:
On https://github.com/python/cpython:
- Enable
*tag protection - Add
*branch protection rule-
Jason'sPR closed/branch (re)moved for nowgh-93963/remove-importlib-resources-abcsin PR gh-93963: Remove deprecated names from importlib.abc. cpython#94528 which started the discussion in Recommend a procedure for branch management #475 in the first place. I've pinged the PR. -
Irit'sPer Irit I deleted the branchrevert-15688-bpo-38031-_io-FileIO-opener-crashhttps://github.com/python/cpython/tree/revert-15688-bpo-38031-_io-FileIO-opener-crash , which does not appear to be connected to any PR. -
Raymond's https://github.com/python/cpython/tree/batched_recipe_tuple , which is an apparent duplicate of the branch on his own fork in PR [3.11] GH-98363: Update batched() recipe in docs to match 3.12 cpython#100323 . I've pinged him on the PR inquiring about the situation.Per Raymond I deleted the branch
-
- Update
*branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branches
On https://github.com/python/peps:
- Enable
*tag protection - Enable
gh-pagesbranch protection - Add
*branch protection rule-
Petr'sencukou-patch-pep620in PR PEP 620: Withdrawn rather than rejected peps#2923, which is approved and I'll merge it shortly if there are no objection. -
Ezio'spep562-repr-examplein PR PEP 562: Fix example by adding repr in an error message peps#2928, which is likewise approved and can be merged shortly if not objections.
-
- Update
*branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branches
On https://github.com/python/devguide:
- Enable
*tag protection - Add
*branch protection rule (withRestrict who can push to matching branches,Restrict pushes that create matching branches, andAllow deletion)-
Guido'sMoved to Guido's forkinternals-interpreterhttps://github.com/python/devguide/tree/internals-interpreter, which does not appear to be connected to a PR. @gvanrossum , do you think you could move that to your fork?
-
- Update
*branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branches- First, clean up the four accidentally pushed and already-merged or abandoned branches
On https://github.com/python/core-workflow:
- Enable
*tag protection - Rename
mastertomain(see Rename default branch from "master" to "main" #492) - Add a branch protection rule for
*(with settings as screenshot below)-
Ezio'sMoved to fork and re-openedclabot2pyorgin PR Updatecla-botentry after moving it to thepythonorg #470 which is pending action. I've pinged the PR. -
Hugo'sMergedupdate-triage-request-templatein PR Update fields for triage membership template #491 which should be mergable quickly.
-
EDIT: It seems python/bedevere also has the
Writerole enabled for triagers, but none of the protections (as I can push and merge as a triager). Assuming we want to keep it that way, the following steps need to be implemented:On https://github.com/python/bedevere:
- Enable
*tag protection - Add the following to the branch protection rule for
main:Restrict who can push to matching branches, and theCore DevelopersGitHub Team added to the list of teams allowed to push - Add a branch protection rule for
*(with settings screenshot below):-
https://github.com/python/bedevere/tree/remove-bpo-autolink in closed/outdated PR Remove autolink to bpo ticket. bedevere#196 — @Mariatta @brettcannon can it just be removed now?Removed as outdated/PR closed. -
https://github.com/python/bedevere/tree/feature/python-pre in PR Build against pre-releases then stable releases. bedevere#472 -
https://github.com/python/bedevere/tree/fix-gh-workflow in PR Avoid running CI twice on PRs. bedevere#461
-
EDIT: After the previous issues with GitHub were fixed, the settings for the
*branch protection rule should look exactly like this (with nothing else checked), as tested and confirmed on another repo:
- ✔️ I have the expected added permissions listed in
Still pending a
core-workflowadmin orpythonorganization owner to complete the changes there (I've pinged @Mariatta with all the details, but she might be away for the holidays), and @gvanrossum merging, moving or deleting hisinternals-interpreterbranch on the devguide, and then having an admin (maybe Brett?) take appropriate action there.However, I've discovered a new issue: apparently I (and I'm assuming all triagers) now have write permissions on the Bedevere repo, but none of the branch protection steps were implemented, so I (and I assume any other triager) can merge PRs, push branches, etc. willy nilly. That should be fixed by an admin there as well...maybe @Mariatta can help?
Specifically, either triagers should be set back to
Triageaccess, or the following changes should be implemented:- Add a branch protection rule to the
mainbranch (if not already present), and enableRestrict who can push to matching branchesandRestrict pushes that create matching branchesand theCore DevelopersGitHub Team (and/or any others) added to the list allowed to push - Enable Tag Protection for all tags (
*) so triagers (or any non-RMs) cannot create or delete release tags - Add a branch protection rule
*matching all branches with the same options enabled, as well asAllow deleting matching branchesat the bottom (or, alternatively, merge, move or delete the remaining upstream feature branches, which allows uncheckingAllow deleting matching branchesand not needing to add anyone to the list allowed to push, and instead checkingLock branchandDo not allow bypassing the above settings
Thanks!
- Add a branch protection rule to the
It seems like Mariatta doesn't in fact have admin on Bedevere, but she mentioned @brettcannon might be able to help out (and I assume on the devguide as well, once Guido's branch is dealt with)? Or anyone who is an Python org owner.
I don't have admin roles on Bedevere, but I do on the devguide. I added the tag protection on the devguide. Just let me know when you're ready for the branch protection.
Reacted by C.A.M. GerlachThanks @brettcannon ! As Guido took care of the last branch, looks like we're gogo on the final step for all-branch protection on the devguide, so all you need to do is on the devguide branch protection settings page:
Add a new branch protection rule for
*matching all branches with theRestrict who can push to matching branches,Restrict pushes that create matching branchesEDIT: plusLock branchandDo not allow bypassing the above settingssettings enabled (to avoid admins unknowingly pushing branches to upstream without deliberately creating a new branch protection rule, which would be required first in case of any deliberate new branch, and would otherwise prevent them from force-pushing or removing their own feature branch)Oh, and could you also double-check thatChecked and fixed by Jelle, thanks!Lock branchandDo not allow bypassing the above settingsare enabled on the PEPs repo? Thanks!- added a commit that references this issue
on Jan 8, 2023 Thanks @brettcannon ! As Guido took care of the last branch, looks like we're gogo on the final step for all-branch protection on the devguide, so all you need to do is on the devguide branch protection settings page:
Done!
Reacted by C.A.M. GerlachReacted by C.A.M. Gerlach and Ezio MelottiHey, so good news. It looks like in GitHub's latest branch protection changes they fixed how the rules interact, so the previous issue where admins were still able to accidentally push new arbitrary feature branches to upstream (that were then read-only and not deletable for everyone else) has now been fixed, with the right combination of settings.
I re-tested the same combination settings that we previously developed and tested to implement this, and we can confirm that this combination of settings for the
*rule (that applies only to new branches being pushed) now appears to work and successfully prevents accidental upstream pushes creating new arbitrary branches, while not affecting other branches:
After doing any mentioned pre-requistie steps in the the checklist above could repo admins/org owners ensure the above-screenshot settings are active for the
*branch protection rule for thecpython,peps,devguideandcore-workflowrepos?Also,
core-workflow(andbedevere, which was not originally listed here) has theWritefor triagers enabled but without the other required pre-requisites, so we are still waiting on a repo admin to take the other steps listed above too, andcore-workflowis also still pending amaster->mainrename (there's an open PR to make the content changes, we just need an admin to actually do the settings change), which should be done first. Thanks!FYI I'm going to bow out of doing any of the tweaks, but you can probably ask @ambv to handle most of this.
Thanks, I'll ask him 👍
Thanks to @ambv , as far as we're aware all the outstanding tasks are completed, so (finally) closing this now! Thanks all!
Reacted by Hugo van Kemenade and Oleg Iarygin
GitHub released custom repository roles yesterday.
It might be useful if Triagers could edit issue/PR titles, edit projects from the issue page, and potentially approve Actions workflows for new contributors.
cc: @ezio-melotti @brettcannon @JelleZijlstra @CAM-Gerlach
A