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.

Sign up for GitHub

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

Jump to bottom

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

Closed
87 tasks done
dblock opened this issue Jan 10, 2023 · 35 comments
Closed
87 tasks done

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

dblock opened this issue Jan 10, 2023 · 35 comments
Assignees
@dblock

Comments

@dblock
Copy link
Member

dblock commented Jan 10, 2023 •
edited

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?

  • Collaborators in opensearch-project expect transparency in who maintainers on any of the projects are.
  • Maintainers want to be notified (or have the option not to be notified) when PRs are opened.
  • Maintainers need reliable lists to find other maintainers to be able to follow our process of adding new maintainers.
  • Maintainers want to be publicly recognized for their work.

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
Copy link

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
Copy link
Member

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
Copy link
Member Author

dblock commented Jan 10, 2023

@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?

@dblock dblock mentioned this issue Jan 13, 2023
Merged
@dblock dblock mentioned this issue Jan 25, 2023
Closed
@dlvenable dlvenable mentioned this issue Feb 7, 2023
Merged
5 tasks
@dlvenable
Copy link
Member

@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
Copy link
Member Author

dblock commented Feb 8, 2023 •
edited

@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.

  • You cannot have external contributors in a team without adding them to the org, which is not currently possible.
  • Teams are being added left and right to repos, then people are added to teams granting them rights to repos without updating MAINTAINERS.
  • You cannot see team members from the outside.

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
Copy link
Member

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
Copy link
Member

gaiksaya commented Feb 9, 2023

One more use case is here opensearch-project/opensearch-build#3111

This was referenced Feb 13, 2023
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@dblock
Copy link
Member Author

dblock commented Apr 6, 2023

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
Copy link
Member

lezzago commented Apr 6, 2023

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
Copy link
Member Author

dblock commented Apr 6, 2023

@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
Copy link
Member

dbwiddis commented Apr 6, 2023

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

What is the definition of "contribute"?

@seanneumann
Copy link

(thanks to @ananzh)
documentation-website#2878 (PR: opensearch-project/documentation-website#3639 and opensearch-project/documentation-website#3641, Status: In review)
OpenSearch-Dashboards#3426 (PR: opensearch-project/OpenSearch-Dashboards#3792, Status: In review)
opensearch-dashboards-functional-test#526 (PR: opensearch-project/opensearch-dashboards-functional-test#606, Status: In review)
oui#305 (PR: opensearch-project/oui#668, Status: In review)
i18n-plugin#17 (PR: opensearch-project/dashboards-i18n#20, Status: merged)
opensearch-catalog#9 (PR: opensearch-project/opensearch-catalog#11 Status: PR is out but I don’t see a merge button)
oui-docs-cdk#8 (PR: opensearch-project/oui-docs-cdk#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
Copy link
Member

@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
Copy link

seanneumann commented Apr 7, 2023 •
edited

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

@dbwiddis
Copy link
Member

dbwiddis commented Apr 8, 2023

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
Copy link
Member

SuZhou-Joe commented Apr 10, 2023 •
edited

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

@dblock dblock mentioned this issue Apr 10, 2023
Closed
@macohen
Copy link
Contributor

macohen commented Apr 10, 2023

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.

@dblock
Copy link
Member Author

dblock commented Apr 10, 2023

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 opensearch-project/dashboards-notifications#31, but please don't hesitate to open issues for things like these next time and I can always link above.

@dblock
Copy link
Member Author

dblock commented Apr 10, 2023 •
edited

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.

@joshuarrrr joshuarrrr mentioned this issue Apr 20, 2023
Open
ananzh added a commit to ananzh/documentation-website that referenced this issue Apr 20, 2023
@ananzh
Update CODEOWNER to match maintainer
b639585 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.

Issue Resolve
opensearch-project#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh mentioned this issue Apr 20, 2023
Merged
1 task
ananzh added a commit to ananzh/documentation-website that referenced this issue Apr 20, 2023
@ananzh
Update CODEOWNER to match maintainer
07543ae 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
opensearch-project#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
ananzh added a commit to ananzh/documentation-website that referenced this issue Apr 20, 2023
@ananzh
Update Maintainer.md to match CODEOWNER
4625f83 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
opensearch-project#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Naarcha-AWS pushed a commit to opensearch-project/documentation-website that referenced this issue Apr 20, 2023
@ananzh
Update Maintainer.md to match CODEOWNER (#3831)
f9a966a 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
vagimeli pushed a commit to opensearch-project/documentation-website that referenced this issue Apr 25, 2023
@ananzh @vagimeli
Update Maintainer.md to match CODEOWNER (#3831)
f5ba385 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
This was referenced May 1, 2023
Open
Open
vagimeli pushed a commit to opensearch-project/documentation-website that referenced this issue May 4, 2023
@ananzh @vagimeli
Update Maintainer.md to match CODEOWNER (#3831)
ed6463f 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@amitgalitz amitgalitz mentioned this issue May 8, 2023
Merged
1 task
@dblock
Copy link
Member Author

dblock commented May 10, 2023

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

@dblock dblock closed this as completed May 10, 2023
@dblock dblock unpinned this issue May 10, 2023
@peternied peternied mentioned this issue Oct 13, 2023
Closed
harshavamsi pushed a commit to harshavamsi/documentation-website that referenced this issue Oct 31, 2023
@ananzh @harshavamsi
Update Maintainer.md to match CODEOWNER (opensearch-project#3831)
cadd125 
Based on requirement here:
opensearch-project/.github#125

CODEOWNER should include a subset or all maintainers.
Based on the request, update maintainers.

Issue Resolve
opensearch-project#2878

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@prudhvigodithi prudhvigodithi mentioned this issue Dec 1, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dblock dblock

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests