test-infra
test-infra copied to clipboard
kubernetes
[prow] Add support for github teams in OWNERS files
as per title, if a team has a large amount of repositories it is cumbersome to update all the OWNERS files when the membership of the team changes.
https://github.com/kubernetes/test-infra/blame/master/prow/plugins/approve/approvers/README.md#L36 points out erroneously that teams are not audited - but that is not the case as documented
So it would be great if prow added support for teams in the owners file.
How would we would distinguish OWNERS_ALIASES from GitHub teams? Using GitHub teams instead of OWNERS_ALIASES would dramatically increase API token usage for all plugins that use OWNERS files unless the github cache is deployed (which is something we cannot assume) so I'd be hesitant to accept this change unless we can demonstrate that that is not a concern. Even if this is feasible I won't have time to work on this Q4 so I'm unassigning myself. /unassign
how would we would distinguish OWNERS_ALIASES from GitHub teams
I may be wrong but today aliased and users have the same form in the OWNERS file ( - string)
approvers:
- alice
- bob # this is a comment
reviewers:
- alice
- carol # this is another comment
- sig-foo # this is an alias
There is no difference here between sig-foo and alice for example.
A team in github is referenced in the form org/team so it can be distinguished by virtue of having a slash. Now I do not find a definitive spec on what is or is not allowed in an alias so it could possibly contain arbitrary characters including the slash but then I would turn the table and ask how you distinguish today between users and aliases? Isn't that the exact same problem that is already solved?
either the string is an alias or it is not. As users can not contain a / it logically follows that if the entry is not an alias and contains a slash it should be treated as a team.
The API token usage is a valid concern but given this would be opt in (no one is making anyone use teams, and you can tell up front if the thing smells like a team before even calling the API) it should be possible to implement with no impact on anyone not using teams in an OWNERS.
it should be possible to implement with no impact on anyone not using teams in an OWNERS.
Only if the feature were disable-able. If one repo uses this, the token usage affects everyone on that Prow deployment.
I also dislike this a little bit, because other than hook, prow nominally supports other git hosts (mainly gerrit atm) and teams is a github thing, whereas owners and aliases are quite portable.
Teams have some major UX downsides for contributors too, non-org members can't @ mention them last I checked, and more importantly cannot view who is in the team at all. ALIASES are public and we tend to prefer checking in config.
One thought: A different way of doing this is to have a mechanism to keep aliases synchronized with teams. Perhaps something for peribolos @fejta
Really interested in using this going forward. I think it would help simplify the fact that we have too many OWNERS files in too many places that end up going stale.
My evil plan is something like:
- add support github teams to approvers plugin, dereference/expand the team when it comes to choosing who to assign (aka do not consider teams valid assignees or reviewers, only their members)
- manage those teams from a single repo (kubernetes/org) and make that a very self-service process
- link to the files that drive those teams in approve bot comments
- suggest to the community that we prefer the use of teams rather than named individuals in owners files
teams are not audited - but that is not the case as documented
This is only viewable to admins of a given GitHub org, we prefer public transparency. Further, there is no way to know the reasons people are added or removed based off of the audit log. PRs help with that.
add support github teams to approvers plugin, dereference/expand the team when it comes to choosing who to assign (aka do not consider teams valid assignees or reviewers, only their members)
manage those teams from a single repo (kubernetes/org) and make that a very self-service process
FWIW, you do not need github teams or the github API to do this, instead you can let the approvers package be aware of this central teams config ...
Aliases have been something of a UX pain point. It's difficult to discover the set of owners given an OWNERS file with an alias because:
- aliases have the same form as user names
- you need to read the lengthy OWNERS files docs and locate the OWNERS_ALIAS file and cross reference alias entries with the OWNERS_ALIAS file
Regarding API call cost: @cjwagner we cache github API calls now anyhow right?
As mentioned above GitHub teams are distinct from user names because they have <org>/<team> and github user names cannot have / in them.
I think this also alleviates my concerns about using this with non-github hosts, as none that I'm aware of have usernames with / in them.
This is only viewable to admins of a given GitHub org, we prefer public transparency. Further, there is no way to know the reasons people are added or removed based off of the audit log. PRs help with that.
This is now solved by way of peribolos + github.com/kubernetes/org. There's an audit log (the git history / PRs) and publicly readable files containing team membership.
Even if we don't do this feature, I think we should consider at least making aliases visually distinct from usernames in some way. Perhaps at minimum we could allow and encourage @username in OWNERS files and start using that to distinguish users from aliases. WDYT @cjwagner ?
Yeah ghproxy will make this pretty much free in terms of rate limit, but it could be very expensive without. I think thats fine as long as we mention that in the OWNERS file documentation.
Allowing the use of @ as a hint that an entry is a username SGTM. If we do that we may want to supply a simple script to add @ to all entries that aren't found in OWNERS_ALIASES.
Yeah ghproxy will make this pretty much free in terms of rate limit, but it could be very expensive without. I think thats fine as long as we mention that in the OWNERS file documentation.
we should also maybe make the feature opt-in still to be safe :thinking:
Allowing the use of @ as a hint that an entry is a username SGTM. If we do that we may want to supply a simple script to add @ to all entries that aren't found in OWNERS_ALIASES.
sgtm!
I started to implement @ support but...
https://yaml.org/spec/1.2/spec.html#id2774157
@ as the leading character in an unquoted string is unused but reserved in yaml :/
EDIT: we can quote them all I guess..
tagging the issue I use for anything related to maintenance or use of OWNERS ref: https://github.com/kubernetes/community/issues/1808
I really, really like the idea of a tool to sync github-teams into the OWNERS_ALIASES file in each repo it manages. That would massively simplify installation in orgs which have a lot of non-OSS stuff, which in turn might lead to more love for this tooling. I guess this tool would have to open PRs, like autobump does? Would that be appropriate inside peribolos, or a new plugin?
I think we should just support github teams in the files instead of PR-ing ~200+ repos constantly.
I mentioned this during SIG chair/TL meeting today. I see a lot of PR's that refresh OWNERS_ALIASES files in multiple repos to basically match GitHub Team membership. Supporting GitHub Teams directly as another kind of alias would eliminate this need.
At this point it's safe to assume ghproxy's presence to mitigate rate limit issues, it's part of the starter manifest now
One problem with teams ... They're pretty much completely unusable for non-org-members, if you are not in an org then you cannot:
- list / view the teams in an org to discover teams
- view the members of a particular team in an org
@mentiona team in an org (github will treat it like a username that does't exist, I'd forgotten https://github.com/kubernetes/test-infra/issues/3609)
Pretty much if you're not a member of an org, the teams in that org are not available to you. That seems extremely problematic for finding / assigning approvers etc.
https://github.com/kubernetes/org/issues/2490#issuecomment-776293399
Plugins could solve those issues, for example /auto-assign /auto-assign-more, /rotate-assignees, /add-approver-for some/random/file etc.
The primary reason people need to see who is in a team is so that they can do something with it, right? AKA @bentheelder this is ready for another look, /assign @fejta \n please approve these build changes
Solve the problem more explicitly, in a way that should help better distribute reviews (aka it would evenly distribute between assignees rather than always my always doing /assign @bentheelder because I believe you'll review fast.
FWIW, I've implemented a peribolos -> OWNERS_ALIASES bit of automation in Knative: https://github.com/knative/community/tree/main/mechanics/tools/gen-aliases, but having native GitHub teams support would be preferable, because we still need to sync the OWNERS_ALIASES to about 60-80 repos.
@antonylucisano this issue will not autoclose (it has lifecycle/frozen) but commenting "bamp" ("bump"?) will not lead to an implementation either.
Most of the people that have previously commented on this issue work on other things now (myself included).
Someone needs to flesh out a proposal and bring it to SIG Testing for discussion.
@BenTheElder thanks for the info, we're looking at writing a proposal ourselves. Will hopefully have something to bring to you. Or have changes to PR
Any plans to potentially pick up working on this feature. Really a pain to have to script updating this OWNER_ALIASES file in thousands of repos at our company when companies already have defined teams in Github.
