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

Valid Owner Checker should report if the owner doesn't have Write access #157

Open
helixliam opened this issue May 18, 2022 · 1 comment · May be fixed by #222
Open

Valid Owner Checker should report if the owner doesn't have Write access #157

helixliam opened this issue May 18, 2022 · 1 comment · May be fixed by #222
Milestone

Comments

@helixliam
Copy link

Currently, the Valid Owner Checker performs the following checks:

1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address).

2. Check if a GitHub owner has a GitHub account

3. Check if a GitHub owner is in a given organization

4. Check if an organization team exists

In Step 3, instead of checking whether the owner is a member of the organization, the Valid Owner Checker should check whether the owner has Write or Admin access to the repository.

Reasons

This is consistent with the native GitHub codeowners validation check that appears in the GitHub web interface. Only a code owner with Write access or greater can approve PRs: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews

This change would lead the codeowners validator status check to report failure under the following scenarios, consistent with the native validation check:

  1. A code owner is added who does not have access, or who only has Read access
  2. A code owner's access is removed (explicitly or when their team membership is changed)

In the GitHub native codeowners validator, each of these would result in the following error:

image

Clicking the ellipsis shows more details about the error.

image

@iamstarkov
Copy link

this would be lovely

@mszostok mszostok added this to the v0.8.0 milestone Aug 17, 2022
infinisil added a commit to NixOS/org that referenced this issue Apr 26, 2024
We shouldn't use personal access tokens, instead we created a GitHub App
with read-only access to just this repository.

While codeowners-validator supports GitHub App authentication,
the same cannot be said for the hacky script I wrote because there was no support
for checking write access: mszostok/codeowners-validator#157

Instead of trying to hack the script more to make it work with GitHub App authentication,
I decided to implement it into codeowners-validator itself: mszostok/codeowners-validator#222

Because it's not merged/released yet, we need to build it ourselves,
so I added some Nix to do that reproducibly.
infinisil added a commit to NixOS/org that referenced this issue Apr 26, 2024
We shouldn't use personal access tokens, instead we created a GitHub App
with read-only access to just this repository.

While codeowners-validator supports GitHub App authentication,
the same cannot be said for the hacky script I wrote because there was no support
for checking write access: mszostok/codeowners-validator#157

Instead of trying to hack the script more to make it work with GitHub App authentication,
I decided to implement it into codeowners-validator itself: mszostok/codeowners-validator#222

Because it's not merged/released yet, we need to build it ourselves,
so I added some Nix to do that reproducibly.
infinisil added a commit to NixOS/org that referenced this issue Apr 26, 2024
We shouldn't use personal access tokens, instead we created a GitHub App
with read-only access to just this repository.

While codeowners-validator supports GitHub App authentication,
the same cannot be said for the hacky script I wrote because there was no support
for checking write access: mszostok/codeowners-validator#157

Instead of trying to hack the script more to make it work with GitHub App authentication,
I decided to implement it into codeowners-validator itself: mszostok/codeowners-validator#222

Because it's not merged/released yet, we need to build it ourselves,
so I added some Nix to do that reproducibly.
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 a pull request may close this issue.

3 participants