steering-council icon indicating copy to clipboard operation
steering-council copied to clipboard

Final Proposal for SC Consideration: Enabling GitHub features for triagers without commit rights

Open CAM-Gerlach opened this issue 1 year ago • 2 comments

Motivation

At present, triagers are unable to use a number of GitHub features that would be useful to them in their role in supporting the core devs, including:

  • Some previously possible on BPO (editing titles, several metadata elements, linked PRs, etc)
  • Several currently listed in the Triaging page of the devguide (linking PRs to issues, fixing titles, setting the project field, etc)
  • Many/most interactions with newly adopted and increasingly relied-upon features such as Projects and the Task Lists
  • A number of others (e.g. re-run GH action jobs and approve them for new contributors, transfer issues between repos, auto-ping on PRs touching specific files, fixing formatting in issue reports/comments, hiding spam comments, etc).

Finding a way to allow such without repo access would improve their ability to contribute, reduce load on core devs and help the project operate more efficiently and effectively.

Rationale

As discussed in detail on python/core-workflow#460, customizable granular role permissions would be the ideal solution, but the current limited customization does not provide any of these, and it seems from what we've been told, it is unlikely to be added in the near term. However, using branch protection, we have devised and tested a viable workaround that would grant triagers these GitHub-level permissions, but not actually touch the repository itself or impact the current core dev or RM workflow.

Specification

  • Enable the Restrict who can push to matching branches and Restrict pushes that create matching branches branch protection options for all branches
  • Add the Core Developers GitHub Team to the list of people/teams allowed to push to >= bugfix branches
  • Add a branch protection rule * matching all branches with the former two options enabled, and optionally with the Core Developers team allowed to push (see python/core-workflow#475 for discussion which concluded it wasn't, and not doing so would follow existing policy and avoid unintentional mistakes).
  • [Already done] Enable Tag Protection for all tags (*) so triagers (or any non-RMs) cannot create or delete release tags
  • Set triagers to the Write role on the cpython repo

The net result is:

  • Triagers can use an expanded set of GItHub features mentioned above, but cannot do anything that actually touches the repo itself (create/delete tags or branches, commit/push to the repo, or merge PRs), the fundamental purview of a core developer.
  • Core developers would see no change to their current permissions/workflow
  • There would be no change for non-org-members or non-triagers
  • RMs would see a very small one—instead of enabling/disabling Restrict who can push to matching branches and adding themself, they would instead just add/remove the Core Developers GitHub team to/from the list.
  • Bedevere's messages (e.g. awaiting core review) are not based on Role, so they should be unaffected as well.

At present, this is proposed specifically for the CPython repo, but in the future, this could be applied to additional org repos if its primary maintainers agree it is useful and appropriate. I propose we implement this after the 3.11.0/3.12.0a1/etc. release in two weeks, since we're so close now, to avoid even a remote chance of any disruption.

I will document the one small RM change in PEP-101 as requested by RMs; if we need to write up documentation for this elsewhere, I'm happy to take care of that as well.

Security considerations

After careful consideration, this should have relatively minimal security impact:

  • Triagers are already trusted users with access to the private Discord, and this does not allow them to modify the actual repo/code/tags/etc. in any way.
  • While some non-essential permissions are enabled, none are particularly destructive or irreversible.
  • Like any org member, in the very unlikely event a triager were to abuse these new permissions, these actions are generally highly visible and reversible, and they could be removed from the team or the org.
  • Once implemented (with setting the Write role as the last step), this approach is generally "fail-safe"; if RMs inadvertently neglect to re-add the core dev team to the list in branch protect, core devs will not be able to push/merge (just as if they were to currently forget to turn the rule off). rather than triagers being able to do so
  • The one failure mode would be RMs accidentally disabling the rule completely instead of just adding back the Core Developers team; the RMs have been briefed and run through the new workflow, this would be added to PEP 101 and if it were to temporarily happen, it would be limited to the relatively small number of trusted triagers (who would quickly report it).
  • If necessary, this could be disabled again in all cases with a single setting (the Role for the Triage team).

Discussion

This follows detailed discussion on python/core-workflow#460 , where after working through a few final points, there was consensus in favor of the change and no major objections; this was announced on Discourse and the core dev Discord, where it earned a number of :+1: reactions and no concerns, and discussed in person at the Python sprints with all four current RMs (@ned-deily, @ambv , @pablogsal and @Yhg1s ) and the four present SC members, and all were supportive with no major concerns.

References

For a more detailed summary, see the Discourse post, and for the complete discussion, see python/core-workflow#460 .

CAM-Gerlach avatar Oct 09 '22 19:10 CAM-Gerlach

Thanks! I added this to the agenda.

encukou avatar Oct 10 '22 08:10 encukou

Accepted. — Petr, on behalf of the SC

@ambv, as a repo owner, could you implement this?

encukou avatar Oct 10 '22 18:10 encukou

Now that the release is over with, maybe its time to go ahead with implementation? @ambv , I can be available synchronously to help test and check that its working as intended if that's helpful. The key thing to avoid any permissions leakage is to make sure the steps are implemented in order—the only step that actually grants people more rather than fewer permissions is the last one.

CAM-Gerlach avatar Nov 02 '22 17:11 CAM-Gerlach