search

opensearch-project / .github

Provides templates and resources for other OpenSearch project repositories.
Apache License 2.0
28 stars 71 forks source link

[META] Baseline MAINTAINERS, CODEOWNERS and external collaborator permissions #125

Closed dblock closed 1 year ago

dblock commented 1 year ago

What/Why

What are you proposing?

There are some common differences across repos in opensearch-project when it comes to maintainers, codeowners and repository permissions.

  1. The MAINTAINERS.md list is not the folks actively engaged in a project.
  2. PRs get approved by folks that are not in the MAINTAINERS list, because permissions aren't in sync.
  3. CODEOWNERS doesn't exist, so maintainers don't always get properly notified about pull requests or their list doesn't appear in reviewers when contributors make pull requests.
  4. CODEOWNERS is out of sync, and does not include a subset, or all maintainers.
  5. Actual repository permissions may not include folks in MAINTAINERS as external collaborators.
  6. Repo permissions may give some groups access to more repos than they are a member of.

The proposal is to correct and align on all the issues above in a single campaign.

What problems are you trying to solve?

What will it take to execute?

First, baseline MAINTAINERS if you've never done that.

  1. Email/reach out to current maintainers and ask them whether they want to still be a maintainer.
  2. Check the list of people in MAINTAINERS.md to include only active maintainers.
  3. Add/move non-active maintainers to an emeritus section.
  4. Add active maintainers to CODEOWNERS.
  5. Remove any broad teams from CODEOWNERS.
  6. Make a PR with the changes above, make sure CODEOWNERS is green.
  7. Once the PR is approved, ask an org admin or an admin delegate (e.g. @bbarani) to sync the repo permissions, removing groups and adding individual contributors. Let's promote some new folks to this role to help, too.
  8. Merge the PR.
  9. Maintain the MAINTAINERS.md and CODEOWNERS.md accurate.

Verified

wbeckler commented 1 year ago

This looks great to me. Reminder to maintainers: review https://github.com/opensearch-project/.github/blob/main/RESPONSIBILITIES.md#becoming-a-maintainer in order to execute these activities according to the agreed policies.

CEHENKLE commented 1 year ago

So if the problem is transparency, adding people as "collaborators" does not solve the problem.

AFAIK, Only admins can see the "setting" tabs where collaborators live, so people will have to trust that codeowners/maintainers is correct (as opposed to today where org member can at least inspect the teams). Put another way, you haven't added a mechanism to codeowners/maintainers up-to-date, and you've reduced the number of people who can audit it.

The benefit of teams is that we can make someone a "team maintainer" (with the ability to add and remove team members) without giving them any other additional permissions on the repo. The unintended consequence for moving to collaborators is that you have have to be an admin to add folks. Because of all the other things admins can do, I want to keep the number of admins down to a very small number.

Now if the problem we're solving is "maintainers are getting added, and we want to slow that process down and add a gate to it" (which is a reasonable problem), then making it an admin-only function will do that.

Now an interesting thing would be to say the ONLY way you can get added as a collaborator is if you're in codeowners/maintainers MD (e.g. tooling that automatically syncs one to the other). There's tooling out there to remove folks, so presumably we could have tooling to add folks as well...?

dblock commented 1 year ago

@CEHENKLE Don't you have to be a member of the org to be added to a team? Assuming that's possible, isn't that the same problem of admin gating?

dlvenable commented 1 year ago

@dblock , What is the motivation for using a list of maintainers in the CODEOWNERS file rather than the team identifier? The current approach has one fewer place to have to worry about when updating the current maintainers list.

With this change, there are three things to change: 1) MAINTAINERS.md, 2) CODEOWNERS, 3) Team membership

dblock commented 1 year ago

@dblock , What is the motivation for using a list of maintainers in the CODEOWNERS file rather than the team identifier? The current approach has one fewer place to have to worry about when updating the current maintainers list.

Teams are useful when granting r/w access to several repos, which shouldn't be allowed because each repo has a separate list of maintainers. We have 70 (!) teams rn. A team called clients has 28 members that have r/w access to 19 repos.

So I propose we get rid of teams to treat org members the same way as external collaborators.

The nice thing about teams is that you can have admins of a team without being an admin of a repo. But the above arguments make it quickly difficult to manage.

With this change, there are three things to change: 1) MAINTAINERS.md, 2) CODEOWNERS, 3) Team membership

I propose to get rid of teams altogether.

CODEOWNERS can be further refined to have owners of parts of the repo.

dlvenable commented 1 year ago

Thanks for the detailed explanation! The current problems make sense and I understand why repo permissions can help resolve this.

With this change, there are three things to change: 1) MAINTAINERS.md, 2) CODEOWNERS, 3) Team membership

I propose to get rid of teams altogether.

We will still have three places to change. Instead of the Team membership, we need to update the repo permissions. But, I understand the problem now so I'm onboard with this.

gaiksaya commented 1 year ago

One more use case is here https://github.com/opensearch-project/opensearch-build/issues/3111

wbeckler commented 1 year ago

Given that the rules on maintainers are repo-wide and consistent, could we get rid of codeowners and modify settings so that it is no longer needed?

dblock commented 1 year ago

Given that the rules on maintainers are repo-wide and consistent, could we get rid of codeowners and modify settings so that it is no longer needed?

What are you proposing specifically?

dbwiddis commented 1 year ago
  1. Once the PR is approved, ask an org admin or an admin delegate (e.g. @dblock) to help you sync the repo permissions, removing groups and adding individual contributors. Let's promote some new folks to this role to help, too.

I've removed all groups except the project-wide "admin" group. Is that intended to be left, or is it included in "removing groups" as the repo maintainers won't have visibility to update the permissions if that (org level) group changes.

wbeckler commented 1 year ago

Given that the rules on maintainers are repo-wide and consistent, could we get rid of codeowners and modify settings so that it is no longer needed?

What are you proposing specifically?

I'm proposing that either:

  1. We have a github action that copies the contents of CODEOWNERS into Maintainers.md or vice versa, or
  2. We get rid of CODEOWNERS and change the branch protection settings to rely on the maintainers list (if possible), or
  3. We get rid of Maintainers.md and point the CONTRIBUTING.md to the CODEOWNERS as the list of maintainers.

The idea is to DRY these two files.

dbwiddis commented 1 year ago

I'm proposing that either:

  1. We have a github action that copies the contents of CODEOWNERS into Maintainers.md or vice versa, or

CODEOWNERS has a very specific format that doesn't include fields we have in maintainers such as affiliation. Copying the other way requires a lot of parsing. I'm not sure either file changes often enough that the juice is worth the squeeze on automation.

  1. We get rid of CODEOWNERS and change the branch protection settings to rely on the maintainers list (if possible), or

CODEOWNERS is tightly integrated into GitHub and is directly used in branch protection rules. For example, our repo requires a PR be approved by a code owner. It's not really practical/possible to include such permissions with another file.

  1. We get rid of Maintainers.md and point the CONTRIBUTING.md to the CODEOWNERS as the list of maintainers.

Reasonable if we don't want to include "Affiliation", but also drops the notion of "emeritus" maintainers or ability to acknowledge contributors who are no longer active but are welcome to return from a hiatus.

wbeckler commented 1 year ago

I agree with @dbwiddis and have no further ideas.

dblock commented 1 year ago

@wbeckler Do you think we can close this or do you still think we should do something?

anasalkouz commented 1 year ago

Check the list of people in MAINTAINERS.md to include only active maintainers. Add/move non-active maintainers to an emeritus section.

What is the criteria to define someone as an active or not

Add active maintainers to CODEOWNERS.

Not sure what could be the case where you have someone in MAINTAINERS.md but not in the CODEOWNERS. if there is no significant difference and mostly they will be an identical list, then we why we need to maintain 2 lists. How to make sure CODEOWNERS include a subset or all maintainers. @dblock

dbwiddis commented 1 year ago

What is the criteria to define someone as an active or not

In our case we moved someone to Emeritus in MAINTAINTERS but removed them from CODEOWNERS.

then we why we need to maintain 2 lists

CODEOWNERS is tightly integrated into GitHub PR review requests and branch protection features and contains only usernames (and optionally split them among file types or subdirectories; in most cases we all do *). We can't really get rid of this one.

MAINTAINERS is freeform and i allows additional information such as "Emeritus" and employer affiliation. In theory we could also add more information like specializations within the project for people reaching out. If you want to reduce redundancy you'd eliminate this one.

macohen commented 1 year ago

Is there a way to contact all admins (e.g. create a new issue) for "What will it take to execute?" step #7. Otherwise, I think everyone will just ask @dblock and he'll end up doing all the work there.

joshuali925 commented 1 year ago

I'm working on all new issues for sql repos and just noticed this one. Item 3 (ref) says

Repo permissions only contain individual aliases as collaborators with maintain rights, admin, and triage teams.

I don't have admin access, would I still be able to do this? This page https://github.com/opensearch-project/reporting/settings/access is 404 for me. If I can't check this, who should be doing it?

Edit: didn't realize this issue had more details on steps. in step 7 it does mention admin is required, but i still have the same question as macohen@

dblock commented 1 year ago

Check the list of people in MAINTAINERS.md to include only active maintainers. Add/move non-active maintainers to an emeritus section.

What is the criteria to define someone as an active or not

Add active maintainers to CODEOWNERS.

Not sure what could be the case where you have someone in MAINTAINERS.md but not in the CODEOWNERS. if there is no significant difference and mostly they will be an identical list, then we why we need to maintain 2 lists. How to make sure CODEOWNERS include a subset or all maintainers. @dblock

When you merge a change to CODEOWNERS, GitHub will tell you whether it's valid (a subset of people with actual permissions).

dblock commented 1 year ago

Is there a way to contact all admins (e.g. create a new issue) for "What will it take to execute?" step #7. Otherwise, I think everyone will just ask @dblock and he'll end up doing all the work there.

I think tagging @opensearch-project/admin on a PR and asking them to confirm permission changes is the easiest way.

bbarani commented 1 year ago

Created missing issue for docker-images - https://github.com/opensearch-project/docker-images/issues/21

lezzago commented 1 year ago

Would it be helpful to have a central tool that automatically sends baseline emails to get confirmation on who wants to be a maintainer or not for all the repos?

dblock commented 1 year ago

Would it be helpful to have a central tool that automatically sends baseline emails to get confirmation on who wants to be a maintainer or not for all the repos?

You mean some kind of automation that asks a person whether they want to be a maintainer? Personally I didn't need something like this. In the past month I've nominated 2 maintainers, so had to send a total of 4 emails (2 to existing maintainers and then one to the person I nominated) and open 2 PRs. It's really not that much.

lezzago commented 1 year ago

You mean some kind of automation that asks a person whether they want to be a maintainer? Personally I didn't need something like this. In the past month I've nominated 2 maintainers, so had to send a total of 4 emails (2 to existing maintainers and then one to the person I nominated) and open 2 PRs. It's really not that much.

This is for baselining current maintainers. This way we can automate the baselining procedure to ask all maintainers every so often (the decided interval) if they want to continue being a maintainer. This is to ensure all repos are following the correct baselining procedure.

dblock commented 1 year ago

@lezzago We introduced emeritus to allow maintainers to say that they no longer want to be involved, and to still recognize their past contributions. How would actively removing folks from MAINTAINERS because they went inactive have any material impact?

FYI I am planning to extend https://github.com/opensearch-project/project-tools maintainers audit to verify that permissions match what we have in CODEOWNERS and MAINTAINERS. I definitely think it would be useful to add something like "how long ago did a maintainer contribute last?".

dbwiddis commented 1 year ago

"how long ago did a maintainer contribute last?".

What is the definition of "contribute"?

seanneumann commented 1 year ago

(thanks to @ananzh) documentation-website#2878 (PR: https://github.com/opensearch-project/documentation-website/pull/3639 and https://github.com/opensearch-project/documentation-website/pull/3641, Status: In review) OpenSearch-Dashboards#3426 (PR: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3792, Status: In review) opensearch-dashboards-functional-test#526 (PR: https://github.com/opensearch-project/opensearch-dashboards-functional-test/pull/606, Status: In review) oui#305 (PR: https://github.com/opensearch-project/oui/pull/668, Status: In review) i18n-plugin#17 (PR: https://github.com/opensearch-project/i18n-plugin/pull/20, Status: merged) opensearch-catalog#9 (PR: https://github.com/opensearch-project/opensearch-catalog/pull/11 Status: PR is out but I don’t see a merge button) oui-docs-cdk#8 (PR: https://github.com/opensearch-project/oui-docs-cdk/pull/10 Status: PR is out but I don’t see a merge button) Done and update each one with PR and status. I don’t see a merge button for the last two. Maybe I don’t have a permission.

peternied commented 1 year ago

@seanneumann Here is a cleaner version of your list

Trick is to just reference the url of the issue/PR in list form, e.g.

- https://github.com/opensearch-project/oui-docs-cdk/issues/8
  - https://github.com/opensearch-project/oui-docs-cdk/pull/10
seanneumann commented 1 year ago

Thanks @peternied! I'm a little to quick to copy and paste. :)

dbwiddis commented 1 year ago

Thanks @peternied! I'm a little to quick to copy and paste. :)

But do you have The Key on your desk like I do?

SuZhou-Joe commented 1 year ago

There is no such Baseline issue in dashboard-notifications, @dblock could you please sync up one on this repo as well?

macohen commented 1 year ago

In this scheme, should maintainers should be able to add new maintainers to the repositories without intervention from the opensearch-project/admins? I thought yes, but I just reopened https://github.com/opensearch-project/dashboards-search-relevance/issues/146 because I'm unable to access Settings -> Collaborators on the repo.

dblock commented 1 year ago

There is no such Baseline issue in dashboard-notifications, @dblock could you please sync up one on this repo as well?

I made one in https://github.com/opensearch-project/dashboards-notifications/issues/31, but please don't hesitate to open issues for things like these next time and I can always link above.

dblock commented 1 year ago

In this scheme, should maintainers should be able to add new maintainers to the repositories without intervention from the opensearch-project/admins? I thought yes, but I just reopened opensearch-project/dashboards-search-relevance#146 because I'm unable to access Settings -> Collaborators on the repo.

You're not. It's purposely centralized with the admin team to have some additional controls. I am working on a proposal to change that and have additional admins in each repo. We would need ways to audit permission changes better to support that.

dblock commented 1 year ago

Verified all lists of MAINTAINERS, CODEOWNERS, and actual permissions. Removed all groups with more than triage access from all repos.