Closed AA-Turner closed 1 year ago
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).
Another 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!
I 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
edited by @username
at 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).CODEOWNERS
.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:
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.
@terryjreedy I tried to explain above. Yes, giving them write permission would give them ability to push to the repository by default. However, at the same time we will restrict who can actually push to the repository by using a different setting on GitHub (screenshotted by Cam here: https://github.com/python/core-workflow/issues/460#issuecomment-1156961074). This effectively removes the ability for triagers to push while leaving them with the additional capabilities that this issue lists as desirable for triage.
@ambv Got it. Somehow I had the impression that the special restrictions would only apply to security branches and EOL branches. Overall, I am in favor of anything that saves committer time so it can be applied to reviewing and committing/closing PRs and issues. I am a bit concerned about the possibility of increased friction with an over-enthusiastic triager. (This has happened.) Clear triager docs will help. So would having a couple of people designated as triager advisers who could handle questions and disputes.
Just to make sure: CPython does use tag protection, right?
Just checked, and we don't currently have tag protection rule. Only branch protection rules.
time(triaging) >> time(locking_branches_during_release)
Yup, the core premise this is based on that the increased triager productivity and decreased core dev burden will outweigh the potential costs (including also core dev time spent dealing with overzealous triagers and any other side effects)
It will be a little annoyin* then to the release managers if they use branch locking to remove and re-add proper team lists.
Maybe its worth creating a meta-team of everyone who should have write access, so then its just a matter of adding/removing that one team? Of course, that has a non-zero cost too, so it would depend on whether that cost is worth the savings for RMs (whose time is perhaps the most critical of anyone's).
It is not clear to me what 'the proposal' is.
Sorry if I was unclear; @ambv summarized it well; for more detail, the original proposal is in this comment above
I am a bit concerned about the possibility of increased friction with an over-enthusiastic triager. (This has happened.)
Yeah, that is what I see as the main thing we want to make sure everyone is okay with and prepared for. Actual intentional abuse seems pretty unlikely and is fairly easy to detect, revert and permanently deal with if it were to ever occur, but there are some features that could be misused unintentionally by an overzealous triager if clear guidelines weren't in place.
If we decide we want to go ahead with this. I'd volunteer to help do the grunt work drafting a PR to document the guidelines that are decided upon in the devguide, presumably in a new heading in the Triage Team devguide page.
Additionally, if is seen as necessary, we could have something like a "Trusted Triager" subteam that Triagers get promoted to once both the core devs and the triagers themselves have enough experience to be trusted with understanding and following the guidelines, and only give that team the "Write" role that would enable these GitHub features. This would also allow relegating a triager back to the parent team without removing them from the org completely.
So would having a couple of people designated as triager advisers who could handle questions and disputes.
Indeed, though I certainly don't want to consume too much valuable core dev (much less RM or SC) time. To help minimize that, we could ensure any major areas of uncertainty/confusion were documented in the devguide for posterity, and perhaps even designate certain experienced triagers to "triage" such questions if they have already been answered and documented, only elevating to the core dev level on those that do not already have a clear answer.
Hey @pablogsal @ambv @Yhg1s @ned-deily @Mariatta @terryjreedy any further insight, concerns or prerequisite steps here? Looks like the Discourse thread didn't attract any feedback (negative or otherwise), just four :+1: s
Just checked, and we don't currently have tag protection rule. Only branch protection rules.
That would best be added anyway, I'd think, as only RMs/admins should be able to tag releases, no?
We don't currently use GitHub releases, AFAIK, and have no plans to do so. And I believe it is true that tags can't be pushed by non-admins / non-rms.
Sorry, I may have spoken too soon. Looking at the repo permissions, I agree we should set a wildcard tag protection role covering all tags. Looks like one of the repo admins needs to do that.
Looks like one of the repo admins needs to do that.
Done.
Thanks @ambv . Is there any reason to allow any non-RM core dev to make any arbitrary tag that doesn't happen to start with one of the sequences there? I.e. why not just one rule, *
? That seems to be what @ned-deily was suggesting, if I'm not mistaken, and would avoid any edge cases or ever needing to update it (and make it easier if it ever needs to be temporarily disabled/bypassed, since its only one rule to flip).
To add an additional data point (among a few others have come up recently), looks like the promising new alpha Task Lists feature, e.g. python/cpython#97021 , will have pretty limited functionality for triagers (unless they are the creator of the task list), as they will not be able to add new items to the list, modify existing items, or even create/link a new issue based off an already drafted item, which this proposal would resolve. Similarly, this would allow triagers to update existing checklists/meta issues created by others if needed.
Since its been a bit, anyone have further updates/feedback here? Or should I go ahead and submit this proposal to the SC for further consideration?
After a last round of in person discussion with the RMs and SC members at the CPython core dev sprints, seems like everyone is on board, and its time to formally submit this to the SC. Therefore, I've done so as python/steering-council#144
Since we've waited this long, if the SC gives the formal go-ahead, delaying implementation until after the big Python 3.11.0 final release will avoid any remote possibility of disruption during a critical time.
Thanks for all your help in evaluating, testing and supporting this proposal!
The proposal was accepted by the SC, and @ambv has been asked to implement. Thanks to all involved for your feedback, help and support!
@ambv , let me know if you want a hand testing it or double-checking that all the settings are correct. Once we do, I can submit a PR to update PEP 101 accordingly.
Thanks for doing the detailed work on drafting this proposal and pushing it through Cam!
A
This is done. All members of python-triage now have the Write
permission to python/cpython on Github, however they cannot really push to main
, 3.11
, and 3.10
because I restricted that to core developers (members of python-core). 3.9
and older are already restricted only to the respective RM.
I also made similar changes, but only restricting the relevant main
branch in:
Let me know if you need the same treatment in other repositories.
Thanks again @ambv ! To publicly document my testing so far:
So, of the specified steps in python/steering-council#144:
peps
repo for us PEP editors, and no core devs have reported problems)*
, non-release) branches, still needs to be implemented for those four repos*
tag protection, still need to be implemented for the non-CPython reposSpecifically, to implement Step 3, you can add a new branch protection rule for *
matching all branches with the Restrict who can push to matching branches
, Restrict pushes that create matching branches
, EDIT: and EDIT 2: And Lock branches
and Do 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 the Maintain
and Admin
roles 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 python/core-workflow#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:
*
tag protection*
branch protection rule
gh-93963/remove-importlib-resources-abcs
in PR python/cpython#94528 which started the discussion in python/core-workflow#475 in the first place. I've pinged the PR.revert-15688-bpo-38031-_io-FileIO-opener-crash
https://github.com/python/cpython/tree/revert-15688-bpo-38031-_io-FileIO-opener-crash , which does not appear to be connected to any PR.*
branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branchesOn https://github.com/python/peps:
*
tag protectiongh-pages
branch protection*
branch protection rule
encukou-patch-pep620
in PR python/peps#2923, which is approved and I'll merge it shortly if there are no objection.pep562-repr-example
in PR python/peps#2928, which is likewise approved and can be merged shortly if not objections.*
branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branchesOn https://github.com/python/devguide:
*
tag protection*
branch protection rule (with Restrict who can push to matching branches
, Restrict pushes that create matching branches
, and Allow deletion
)
internals-interpreter
https://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?*
branch protection rule with settings below to avoid admins accidentally pushing unintended (read-only) branches
On https://github.com/python/core-workflow:
*
tag protectionmaster
to main
(see #492)*
(with settings as screenshot below)
clabot2pyorg
in PR python/core-workflow#470 which is pending action. I've pinged the PR.update-triage-request-template
in PR python/core-workflow#491 which should be mergable quickly.EDIT: It seems python/bedevere also has the Write
role 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:
*
tag protectionmain
: Restrict who can push to matching branches
, and the Core Developers
GitHub Team added to the list of teams allowed to push*
(with settings screenshot below):
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:
Still pending a core-workflow
admin or python
organization 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 his internals-interpreter
branch 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 Triage
access, or the following changes should be implemented:
main
branch (if not already present), and enable Restrict who can push to matching branches
and Restrict pushes that create matching branches
and the Core Developers
GitHub Team (and/or any others) added to the list allowed to push*
) so triagers (or any non-RMs) cannot create or delete release tags*
matching all branches with the same options enabled, as well as Allow deleting matching branches
at the bottom (or, alternatively, merge, move or delete the remaining upstream feature branches, which allows unchecking Allow deleting matching branches
and not needing to add anyone to the list allowed to push, and instead checking Lock branch
and Do not allow bypassing the above settings
Thanks!
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.
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:
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: plusLock branch
andDo not allow bypassing the above settings
settings 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 that Checked and fixed by Jelle, thanks!Lock branch
and Do not allow bypassing the above settings
are enabled on the PEPs repo? Thanks!
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!
Hey, 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 the cpython
, peps
, devguide
and core-workflow
repos?
Also, core-workflow
(and bedevere
, which was not originally listed here) has the Write
for 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, and core-workflow
is also still pending a master
-> main
rename (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 :+1:
Thanks to @ambv , as far as we're aware all the outstanding tasks are completed, so (finally) closing this now! Thanks all!
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