lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.49k stars 742 forks source link

CODEOWNERS names users without write access to repo #13469

Open dmcardle opened 2 years ago

dmcardle commented 2 years ago

https://github.com/lowRISC/opentitan/blob/d8af32ff61026d72a7586c66870feaa162335654/.github/CODEOWNERS

The CODEOWNERS file complains about @drewmacrae, @Jacob-Levy , and @milesdai. For each of them, it says, "Unknown owner: make sure $username exists and has write access to the repository".

Skimming Github's "About code owners" doc, I don't understand the reason for the error. The doc implies that users only need read access.

The people you choose as code owners must have read permissions for the repository. When the code owner is a team, that team must be visible and it must have write permissions, even if all the individual members of the team already have write permissions directly, through organization membership, or through another team membership.

Users must have read access to the repository and teams must have explicit write access, even if the team's members already have access.

Possible solutions:

  1. Ignore the errors. Is there any value to listing reviewers without write access? Can they be auto-assigned to reviews?
  2. Grant write access to these users.
  3. Remove the users from CODEOWNERS.
drewmacrae commented 2 years ago

This is a new warning to explain an old problem that otherwise goes on under the hood.

Until users are added to the Committers file, review is weird.

I haven't found Github's documentation useful for understanding the design or intent here.

We have a couple areas where we are uncovered/undercovered on CODEOWNERSHIP at present so we're using the CODEOWNERS file to track intent in addition to steering Github reviews. In the meantime we should probably Ignore the errors.

drewmacrae commented 2 years ago

If the error message read: "Unknown owner: make sure $username exists and has read access to the repository".

Then it would indicate the about code owners doc is correct and we could perhaps do some work to resolve this by granting read access to these users (Option 2 without extending any unintended powers to the parties discussed here). That might also sort out querying for Unknown/non-existant users reviews (in the meantime I can just comment on every PR I review and filter on that instead).

dmcardle commented 2 years ago

Until users are added to the Committers file, review is weird.

Ah, I hadn't seen COMMITTERS yet. Does Github understand this file? Or do we have some automation parsing and controlling Github permissions? COMMITTERS references doc/project/committers.md, but AFAICT that only discusses policy, not mechanism.

If the error message read: "Unknown owner: make sure $username exists and has read access to the repository".

Agreed, that would make more sense!

In the meantime we should probably Ignore the errors.

It's unsatisfying, but I support this choice, at least until we can find an authoritative reference on the semantics of CODEOWNERS. Maybe we should add a comment to the file summarizing/linking this issue.

msfschaffner commented 2 years ago

CC @mundaym @moidx