Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Github Access Structure Overview section #5503

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jonchurch
Copy link
Member

@jonchurch jonchurch commented Feb 24, 2024

Here's a first pass at adding a section about planned Org/Repo access levels.

This doesn't spell out what has changed or will change about who has access. You can see some of the shaking out of dust in this issue expressjs/discussions#132

We should document that somewhere, but I wanted to open a PR to land what I've seen proposed.

I went with my own definitions and opinions here, and specifically am curious if I got the role for Committer correct re: access to a single repo vs all repos in an org.

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated

* Technical Committee – admin permissions across all Express orgs ([expressjs](https://github.com/expressjs), [jshttp](https://github.com/jshttp), [pillarjs](https://github.com/pillarjs)). Highest level of access, owner permissions on repos and orgs. TC members will be added to a github team in each org, and permissions will be granted to the team.

* Project Captain – admin permissions on a specific repo. Permissions similar to TC, but scoped to a specific repo not an entire org. Permissions must be applied manually to a Project Captain user as a result. Creating a github team for this is optional, as the permissions are not applied at the team level.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admin permissions on a specific repo. Permissions similar to TC, but scoped to a specific repo not an entire org.

This feels redundant, maybe just delete the second sentence?

Permissions must be applied manually to a Project Captain user as a result

Is that true? I thought we could manage that with a team where the team was added as admin on the repo.

Copy link
Member Author

@jonchurch jonchurch Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I missed this comment.

For

Permissions must be applied manually to a Project Captain user as a result

Is that true? I thought we could manage that with a team where the team was added as admin on the repo.

I feel like Im spliting hairs here, but you are correct that we can do that. I see benefits to the approach, however, I also see it as creating extra teams to manage and becoming confusing/hard to parse.

Let's say we have 8 different repo captains in a given org and together they captain 12 repos (assuming that of the 60ish repos across all orgs, most repos will not have a captain, but that some people are captains to multiple repos).

That makes 12 unique Github teams. One for each repo, as I understand the suggestion. That's a lot of teams, and will grow as we add more captains. Benefits I see are it's simple enough to onboard someone to the permissions, create the team if it doesn't exist and add admin rights on the given repo to the teama, invite the captain. Offboarding means removing them from the team. The above can be accomplished via individually applied permissions as well, without creating 12 teams of 1. That's basically my whole argument, fewer teams to manage, and if we expect mostly 1 captain per repo the team approach is no less adhoc than individually applied permissions to a user account.

We could also have a situation where a captain steps down without one stepping up. We either delete the team outright, or let it coast as something which can be @ mentioned but has 0 members to see the ping.

Copy link
Member Author

@jonchurch jonchurch Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to bikeshed on this. I will be happy with whatever approach is picked. I brought my own opinions after thinking through the above. I'd prefer to do everything via teams, but I don't think teams map well to the repo captain concept when it comes to applying permissions due to noise/overhead created by teams of 1.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need some more work on this. I left a few initial questions but also I think we need to define what the team names are and likely what specific permissions they have. This should both be a doc for folks to understand what it means but also for us on how explicitly to manage it so we are all on the same page on how to do it.

Contributing.md Outdated

* Project Captain – admin permissions on a specific repo. Permissions similar to TC, but scoped to a specific repo not an entire org. Permissions must be applied manually to a Project Captain user as a result. Creating a github team for this is optional, as the permissions are not applied at the team level.

* Committer – commit bit on a given repo or repos. Permissions must be applied manually. Github team here is optional, but recommended. All members of the org who have a commit bit for one or more repos can be members.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permissions must be applied manually

Not sure what this means. All of this will be manually managed 🤣

Copy link
Member Author

@jonchurch jonchurch Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually here means going into settings in github, selecting a given member from the org and setting their bespoke permissions (write on expressjs/body-parser, expressjs/multer, for example). Removing their perms requires the same manual work.

Contrast that to permission management with teams. TC/owners get admin on everything in the org. You can give those specific perms to a team, and then just add the user to the team. To revoke perms, remove them from the team.

That works well for TC and Triage, who have a homogenous set of perms, but not well for folks w/ ad hoc write access like Committer or Project captain. Which forces you to manually manage perms for individual accounts in the Github UI. It's extra work but I don't think there's a better way.

Here's github docs about these two different approaches which they call "Individually managing access" and "Managing Team Access". These are both very light docs that link to the same place, but they are two distinct access management strategies.

@jonchurch
Copy link
Member Author

jonchurch commented Feb 26, 2024

I think we need some more work on this. I left a few initial questions but also I think we need to define what the team names are and likely what specific permissions they have. This should both be a doc for folks to understand what it means but also for us on how explicitly to manage it so we are all on the same page on how to do it.

I left the team names out because naming is hard! I agree we should define names and specific perms

Github has built in roles that are close to what we want, but we should evaluate if we need deviations. I err on the side of running with the built in perms where possible and adjusting when necessary.

an aside... re: evaluating deviations from the built in roles: e.g. Triagers can review/approve/reject PRs, but the reviews don't count towards merging, as approval by user w/ write permissions is required. It's a nit, but PRs that don't get merged despite being approved by N folks w/o write looks worse than a PR not merging bc it hasn't been approved at all. Taking away approval perms from Triagers is not a priority, but it is an example of a perm we could deviate on and a weak justification. Honestly, this is likely moot bc you can't create custom roles without having Github Enterprise

Here is a summary of the groups I outlined in the PR and what permissions I am suggesting:

  • @expressjs/express-tc (already exists) -- owners role on the org as defined in github org permission docs
  • expressjs/project-captain -- no role assigned to the team beyond base org member role. Dunno if this team would be useful or not as permissions for maintaining a given repo will not be assigned at team level, but individually applied to user accounts. It would give an @expressjs/project-captain mention ability, but I don't think see the need for pinging like 3-4 people who are captains, but not all commiters. Won't block on this.
    • Users will be individually assigned admin repo role as defined in github repo permission docs, per repo they are given project captain status for
  • @expressjs/committers -- no role assigned to the team beyond base org member role (Im assuming that committers have write on a subset of repos and not all in the org, if it's all repos in the org then team based permissions would be appropriate). This one I actually do see as having everyone w/ a commit bit, including TC, Project Captains, and Committers w/ ad hoc write access. So you can just ping everyone with write.
    • Users will be individually assigned write repo role as defined in github repo permission docs, per repo they are given committer status for
  • @expressjs/triagers (already exists) -- triager role on each repo as defined in github repo permission docs

Glossary for Above

  • team -- Github team, a named group of people which can be used to manage permissions/access for people in an organization, sending notifications via @ mention, or requesting review from the team on PRs
  • role -- Github role, a set of built in permissions bundled under a specific name which can be assigned to a user or a team. There are different roles at the Org level and the Repo level
  • "individually applied permission" -- manually adding/removing a role or specific permissions for a given collaborator's user account

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants