Configure sagemath organization Teams, branch protection rules

[mkoeppe]

As sketched in https://github.com/sagemath/trac-to-github/blob/master/docs/Migration-Trac-to-Github.md#proposed-permissions-and-protections

Also,

• Should (temporary) public collaboration branches (like public/.... on Trac) be allowed on sagemath/sage?

[mkoeppe]

@sagemath/core We should probably review membership in the "core" team https://github.com/orgs/sagemath/teams/core/members
Any thoughts on others that should be added to "core" and on retirements of inactive "core" members?
(The "core" team overlaps with but is not identical to the "owner" role in the org.)

[nthiery]

Not being very active myself these days, to say the least. I let you judge depending on the roles of each team whether I should be there or not. Cheers, Nicolas

[kwankyu]

Just to start a discussion: will we have this hierarchy?

1. owners - full power; keep three owners always?
2. core team - what permissions or privileges? release-manager belongs here?
3. reviewer team - what permissions or privileges?
4. ad-hoc team - what permissions or privileges?
5. member - what permissions?
6. non-member - what permissions?

Who can make a team? Or who can assign what permissions to a team?

[jasongrout]

Not being very active myself these days, to say the least. I let you judge depending on the roles of each team whether I should be there or not.

Same for me.

[kwankyu]

This is the document about roles and permissions:

https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/repository-roles-for-an-organization

[kwankyu]

"Read" "Triage" "Write" "Maintain" "Admin" are all available roles (each of which is a bundle of permissions) in our organization (since we cannot make custom roles).

So what we need to do here is to distribute these roles to each team in the hierarchy?

[kwankyu]

?

Team	Role	Number	Number of PRs
Owner	Admin	3	.
Release manager	Maintain	1
Core	Maintain		>= 100
Team manager	Write	1 for each team	>= 50
Member	Triage		>= 10
non-Member	Read

[kwankyu]

?

branch	branch protection rules
develop	require a pull request require 1 approving review require all CI tests pass require branches to be up to date restrict who can push disallow force-push disallow deletion
master(or main or release)	restrict who can push

[kwankyu]

This is the documentation for branch protection rules:

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches

[tobiasdiez]

Like this for the develop branch (with the correct ci workflows obviously)?

(not sure if "lock branch" is overwritten by "restrict who can push")

[mkoeppe]

"require linear history" should be off

[tobiasdiez]

why? squash and an occasional rebase merge seems good enough, or not?

[mkoeppe]

Because it's not the standing practice of this project, and it's not part of the migration to change everything.

[mkoeppe]

Only the release manager team (to be created) should be able to push to develop and master. Not part of this migration to change this.

[mkoeppe]

I've created a new milestone https://github.com/sagemath/trac-to-github/milestone/5 for proposed larger changes to the workflow like this. They will need discussion on sage-devel and with the release manager.

[tobiasdiez]

I think admins and maintainers always have the right to push (merge PRs with passing checks).

[kwankyu]

Only the release manager team (to be created) should be able to push to develop and master. Not part of this migration to change this.

This migration should provide where to start after opening the repo for new issues/prs, though everything is subject to change.

The release manager team seems to coincide with the reviewer team in my table. Right? Or is it above the reviewer team in the hierarchy?

[mkoeppe]

I think the "release manager team" would at the moment only consist of Volker. He needs to be able to merge into develop and master.

"Trusted reviewers", if possible, should be able to edit others' PRs (unless the PR authors have unchecked "allow edits by maintainers".

[dimpase]

Having only release manager pushing to develop would keep things as slow as with trac, as many positively reviewed but unmerged PRs are a source of conflicts.

As is mentioned somewhere, there should be a trunk branch for merging positively reviewed PRs passing CI, which would also serve as a base for the PRs.

In trac model, it's @vbraun 's private branch he uses for building releases - but it should not remain private.

The role of the release manager would be to take care of trunk not going astray, and using it to build releases in develop branch (after each release trunk should get rebased over develop) - and dealing with all sorts of tricky PRs, which keep failing PRs for one or another reason.

[kwankyu]

?

[release manager]        [release manager]   [team maintainer]   
master(release,main) --> develop(trunk) :--> develop-modularization (team)
                                        :--> develop-manifold (team)
                                        :--> develop-combinat (team)
                                        ...

[tobiasdiez]

Having only release manager pushing to develop would keep things as slow as with trac, as many positively reviewed but unmerged PRs are a source of conflicts.

👍

As is mentioned somewhere, there should be a trunk branch for merging positively reviewed PRs passing CI, which would also serve as a base for the PRs.

Isn't the develop branch functioning as this "trunk" branch?

My proposal would be the following for now, which is pretty close to the current trac workflow but decouples the role of "releasing a new version" from the right to merge PRs and thus removes this bottleneck:

• Have a develop branch that serves as the base/target for all PRs, and is the "default branch"
• A group of maintainers is able to merge PRs into develop (and edit PRs, to my understanding you have to have "Maintainer" role in order to edit PRs, and this automatically gives you the rights to merge PRs if all checks are passing)
• From time to time the release manager tags the develop branch to releases a beta from it
• In order to create a stable release, the release manager pushes the develop branch into master/main

Of course, there needs to be a bit of communication between the release manager and the maintainers so that "experimental" PRs are not merged directly before a stable release.

[vbraun]

We should first work on our CI game before changing the merge/release process. PRs should only be merged if they pass on the supported platforms, and right now very few people have a way of checking that. We also might want to nararow down what we support (e.g. is anyone still using 32-bit?). But thats a whole different question for when the github transition is done.

[dimpase]

I agree that before the CI is improved, we should stick to the workflow we have now - i.e. positively reviewed PRs are only merged by the release manager.

Let's at least keep it so until release 9.8 is out the door.

[mkoeppe]

For PRs that only modify src/, I'd say that the current "Build & Test" workflow is sufficient.
I think it's very rare that such a PR introduces a portability problem, including doctest problems on 32 bits.

[mkoeppe]

For now I have simply locked master and develop. Admins can still push.

[tobiasdiez]

Can someone of the admins please setup a Triage team that then can label issues/PRs.

[dimpase]

isn't opening a PR creating a branch? in fact, it does, so I'm a bit worried you blocked opening PRs along the way.

[dimpase]

Atm it's quite silly that one cannot add labels to issues and prs without being on the triage team.

[kwankyu]

If we inherit trac tradition, I think all members should be able to add labels to individual issues/prs.

[kwankyu]

I wonder how fine control is possible for basic permissions of members under our free GitHub license.

[tobiasdiez]

isn't opening a PR creating a branch? in fact, it does, so I'm a bit worried you blocked opening PRs along the way.

No, you normally push to your fork and then create a PR from the branch in your fork. But the branch protection rule only applies to branches created in the main sage repo and not in the fork. (As I've just discovered one can still create branches in the main repo as admin, but then cannot push to them).

I wonder how fine control is possible for basic permissions of members under our free GitHub license.

It's not possible to use any customized permission rule. This is a feature restricted to Enterprise Cloud users: https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/repository-roles-for-an-organization

[dimpase]

one can probably create a GH Action to set up labels based on values in a comment

[dimpase]

one can probably create a GH Action to set up labels based on values in a comment

of course, PR is created on a fork, but it leads to creation of a specially named branch on the base repo.

[kwankyu]

of course, PR is created on a fork, but it leads to creation of a specially named branch on the base repo.

As I understand it, no branch is created on the base repo from a PR.

[kwankyu]

I wonder how fine control is possible for basic permissions of members under our free GitHub license.

It's not possible to use any customized permission rule. This is a feature restricted to Enterprise Cloud users: https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/repository-roles-for-an-organization

That is about a role. Matthias gave minimal permissions to members during the migration. So I guess he could finely control permissions given to members.

[dimpase]

of course, PR is created on a fork, but it leads to creation of a specially named branch on the base repo.

As I understand it, no branch is created on the base repo from a PR.

it is created, its name is pull/ID/head. E.g. the branch of sagemath/sage#34970
can be obtained as follows:

$ git fetch origin pull/34970/head
remote: Enumerating objects: 42, done.
remote: Counting objects: 100% (42/42), done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 42 (delta 30), reused 38 (delta 30), pack-reused 0
Unpacking objects: 100% (42/42), 20.78 KiB | 259.00 KiB/s, done.
From github.com:sagemath/sage
 * branch                  refs/pull/34970/head -> FETCH_HEAD

[kwankyu]

Confirmed. Thanks!

[saraedum]

?
Team Role Number Number of PRs
Owner Admin 3 .
Release manager Maintain 1
Core Maintain >= 100
Team manager Write 1 for each team >= 50
Member Triage >= 10
non-Member Read

If we want to replicate what we had on trac, everybody in the sagemath organization should be able to change labels it seems. However, being a member of "Triage" means that you can also do some other (destructive) actions like closing issues. Should we really worry about that possibility of abuse? It never happened on trac to my knowledge. And randomly closing issues is probably not as bad as scrambling all the labels.

We could use a bot to only let people change labels but then we take away the nice labels interface.

[dimpase]

On trac, only a select group (TracAdmins + few others, about 20 ppl?) was able to close tickets, AFAIK.

One can use something like https://github.com/github/issue-labeler for labelling...

[roed314]

As a short term fix, I think everyone who is a recognizable contributor (e.g. the people who were asking about labels in Matthias' talk) should be added to the Triage team. We can then talk about whether to expand these permissions further.

[roed314]

@dimpase the issue-labeler app you pointed to means that we wouldn't actually get to use the graphical interface for adding labels, which I think is raising the barrier for contributors.

[saraedum]

@dimpase I think it's easier to just have a bot reopen issues if somebody closes them who's not supposed to do that.

[saraedum]

Any thoughts on others that should be added to "core" and on retirements of inactive "core" members?

Should we add everybody who was a trac admin as a start? (I took the liberty to add David Roe so I am not the only one at Sage Days who can do things.)

[dimpase]

sure

[dimpase]

the power of triage users will go up if we start using Discussions: https://docs.github.com/en/discussions/managing-discussions-for-your-community/moderating-discussions
they can convert issues to discussions, and delete discussions.

otherwise, I'd say add to triage team everyone with at least some contribution to sagemath.

[saraedum]

the power of triage users will go up if we start using Discussions: https://docs.github.com/en/discussions/managing-discussions-for-your-community/moderating-discussions
they can convert issues to discussions, and delete discussions.

That also does not sound like a big problem to me. But maybe that's something to discuss again when we enable discussions.

[saraedum]

It turns out that "Triage" is not a possible base permissions for all organization members. So we'd have to explicitly add people to the Triage team.

Let me see if I can cook up a script to synchronize all members to the Triage team (and not run the script yet.)

Then again, what's the process to decide these things here? We just wait for a few people to agree and we can still revert if somebody thinks it's a bad idea?

[dimpase]

I'm just a bit worried about old inactive accounts in Triage team... I'd not just add everyone, yet.

[saraedum]

I see. My idea would be to just replicate things as they were on trac since that should not be controversial in principle. (And then tweak things later.)
Also, the interface to add people to teams is very annoying for adding lots of people. (So writing a script makes this much less annoying…)

[saraedum]

To get things going and be able to work at the Sage Days we added a manually curated (and unfortunately somewhat biased) list of 86 developers to the Triage team.

Using this (unnecessarily inefficient) script:

from github import Github

github = Github("SECRET TOKEN")

org = github.get_organization("sagemath")
triage = org.get_team_by_slug("triage")
members = org.get_members()

import csv

with open("triage.csv") as csvfile:
    curated = list(csv.reader(csvfile, delimiter=","))[1:]
    curated = [row[0] for row in curated]

for member in members:
    if member.login in curated:
        print(f"Adding {member} to Triage team")
        triage.add_membership(member)

[kwankyu]

@sagemath/core

I think that the number of owners should be bare minimum, 2 or 3, as an owner is destructively powerful. GitHub suggests at least 2 and seems to suggest small number too. So I propose @williamstein @mkoeppe @dimpase as only owners of our organization.

and all other current "owners" should be down-graded to "core members" with Admin (or Maintain) role.

[saraedum]

Personally, I would not be afraid of our owners. It's hard to do something destructive accidentally. I'd rather have a larger group of people with privileges, similar to the current trac admins. If we are afraid that accounts might get taken over by bad actors, we could ask the owners to enable 2FA to be on the safe side.

[mezzarobba]

the power of triage users will go up if we start using Discussions: https://docs.github.com/en/discussions/managing-discussions-for-your-community/moderating-discussions they can convert issues to discussions, and delete discussions.

otherwise, I'd say add to triage team everyone with at least some contribution to sagemath.

Apparently, when you convert an issue to a discussion, the issue is locked, not deleted. Github doesn't let you reopen the issue while the discussion exists, but it does after the discussion is deleted.

[kwankyu]

Personally, I would not be afraid of our owners. It's hard to do something destructive accidentally. I'd rather have a larger group of people with privileges, similar to the current trac admins. If we are afraid that accounts might get taken over by bad actors, we could ask the owners to enable 2FA to be on the safe side.

It is not only about accidents. I think that important changes about sage organization/repos should be made after open discussions. Restricting the powers to make the changes to a small number of people will reduce spontaneous changes.

Not that I observed spontaneous changes, and I am not familiar with GItHub permissions system. So my concern may be just imaginary. Anyway, it is not urgent.