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

Update CODEOWNERS #169

Closed
wants to merge 1 commit into from

Conversation

prudhvigodithi
Copy link

Description

Update CODEOWNERS

Issues Resolved

#123

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@prudhvigodithi
Copy link
Author

Please make sure aditjind is invited to the the repo before adding to the CODEOWNERS and MAINTAINERS.md file.
Thank you

Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
@Xtansia
Copy link
Collaborator

Xtansia commented May 10, 2023

The org-wide MAINTAINERS.md states that maintainership status does not expire, but permissions may be removed for security reasons, and that moving to emeritus is a choice by the maintainer. So forcefully moving someone to emeritus seems against our own policy? This was the reason they were left in Maintainer section but removed from CODEOWNERS as it breaks the one-click approval process if they are in CODEOWNERS without permissions (#160)

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #169 (7f4cde2) into main (9383059) will increase coverage by 0.06%.
The diff coverage is n/a.

❗ Current head 7f4cde2 differs from pull request most recent head 4c9eaf5. Consider uploading reports for the commit 4c9eaf5 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   73.47%   73.54%   +0.06%     
==========================================
  Files         398      398              
  Lines       62345    62613     +268     
==========================================
+ Hits        45811    46046     +235     
- Misses      16534    16567      +33     
Flag Coverage Δ
integration 73.54% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

@dblock
Copy link
Member

dblock commented May 10, 2023

The org-wide MAINTAINERS.md states that maintainership status does not expire, but permissions may be removed for security reasons, and that moving to emeritus is a choice by the maintainer. So forcefully moving someone to emeritus seems against our own policy? This was the reason they were left in Maintainer section but removed from CODEOWNERS as it breaks the one-click approval process if they are in CODEOWNERS without permissions (#160)

Are there any reasons to remove permissions? They actually aren't able to approve PRs anymore because they don't have permissions, so does that still make them maintainer? In this case would you mind merging this PR and reaching out to them to re-add them with proper permissions?

@Xtansia
Copy link
Collaborator

Xtansia commented May 10, 2023

The org-wide MAINTAINERS.md states that maintainership status does not expire, but permissions may be removed for security reasons, and that moving to emeritus is a choice by the maintainer. So forcefully moving someone to emeritus seems against our own policy? This was the reason they were left in Maintainer section but removed from CODEOWNERS as it breaks the one-click approval process if they are in CODEOWNERS without permissions (#160)

Are there any reasons to remove permissions? They actually aren't able to approve PRs anymore because they don't have permissions, so does that still make them maintainer? In this case would you mind merging this PR and reaching out to them to re-add them with proper permissions?

Well according to the doc they're still considered a maintainer even if inactive and it's up to them to ask for permissions to be reinstated if they wish to participate again. They did have correct permissions previously, I am unsure who removed them, I don't have repo admin permissions to do so, let alone the org-admin permissions to check the audit log.

I'm just saying we should follow our own policies, or amend them if they don't align with reality.

@dblock
Copy link
Member

dblock commented May 11, 2023

@Xtansia ok, I'm with you.

Permissions were removed because @aditjind used to be a member of the opensearch-project org, then was removed from the org (changed jobs), and thus lost their permissions. I'll make changes to the tool to account for this. If you want to re-add them to CODEOWNERS, please feel free.

Let's close this one.

@dblock dblock closed this May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants