FR: UI should expand which teams (and users) can actually approve something

[iamdanfox]

What happened

A common workflow:

1. I make a PR, all the CI goes green, but the overall light is still stuck at yellow

2. I click through to the policy bot UI to see what I need to do to get my thing merged Unfortunately the UI doesn't tell me who I need to ping.

3. I go back to the main file browser, hit t to browse files, then read the policy.yml file to see which teams are considered maintainers

4. I go to the org homepage

5. Click teams

6. Search for my relevant team, click it

7. Click the members tab

Finally I can go message one of those people.

What should have happened

I'd like this to require less steps - ideally, I'd like the policy-bot UI (and reported commit status summary) to give me a bit more information about who I need to go talk to.

[asvoboda]

What do you think about displaying the text of the approval rule for pending approvals (intentionally omitting skipped or already approved)?

[iamdanfox]

In this case, expanding "0/1 approvals required" to something like "0/1 approvals from: org/team (user1, user2, user3, user4...)" could work?

[bluekeyes]

As a note, the commit status description is capped at 140 characters (and most of those are relegated to title text), so putting more information there has limits, particularly for teams with many users.

Adding something to the UI is more feasible, or adding it in the Checks tab in GitHub, once #46 merges.

[iamdanfox]

Woah posting markdown to the checks UI looks pretty slick!

[tomkozlowski]

+1, just hit this issue as well. Is this issue something currently being prioritized?

[ollie299792458]

+1 for this, by default on github a viewer who can make PRs to a repository cannot see who the admins of a repo are.

In the event of rules containing...

requires:
  admins: true

...there is no useful information in the policy.yml, and steps 3 onwards are not even possible.

This makes it impossible to know who needs to review a PR. The result is that PR authors resort to looking at people who have recently approved PRs or have lots of contributions to the repo. This is not only an additional step (and one that is not obvious, only resourceful users would identify it), but it is also imperfect, as the admins could have changed.

[buffer51]

From #247, I'd like to propose a partial solution when approvers are teams or orgs: add links to the GitHub page for the teams / orgs on the Policy Bot UI.

It doesn't address all the pieces mentioned in this issue: admins or list of users aren't covered (to get around figuring out how to display potential long lists of users and the security implications, since teams & orgs have dedicated pages), it requires clicking into the UI and not seeing it in the checks results.

I do feel it addresses the most common & difficult pattern. Using admins is also common, but GitHub doesn't allow for that information to be visible easily. It's also generally known outside of GitHub who the admins for a particular repository roughly are (or via contributors). On the other hand, teams are most commonly used for repositories that have many files with different approvers, sometimes two layers of approval, which are complicated to understand.

[ollie299792458]

@buffer51 that sounds like a good idea.

However I'd be cautious about downplaying the criticality of the admin approval workflow so much, as there is a workaround for the teams approval workflow (see OP), but aside from the heuristics of contributors and external info there is no workaround for the admin approval workflow, i.e. it is sometimes impossible, which is more difficult that 'difficult'.

[bluekeyes]

This is now partially addressed by #407. While we still don't expand full team membership in the UI, it does show the users, teams, and organizations from the policy and links to them in the GitHub UI.

[ash211]

Thanks @bluekeyes for linking that -- happy to have that improving legibility.

I think with our recent internal migration away from github teams, I now only see that I need approvals from users with the admin or maintain permissions. I think this is what @ollie299792458 was referring to above for admin approval workflow.

Is there a reasonable way to expand this "Users with the permissions" section to some human usernames?

[bluekeyes]

As a note for a possible future implementation (and as context for those interested), there are a few complications that make this more involved than it may seem at first:

1. Showing this information is technically breaking a GitHub security boundary. GitHub requires some level of elevated permission to see the members of a team or their permission levels on a repository, but Policy Bot only requires read permission on the repository to view the UI. Because the policy itself is public, you can infer this information about (some) users by looking at past approvals, but Policy Bot shouldn't unexpectedly make it easy to find this for more users.

   One solution for this is to have a server-level configuration option that enables team and permission expansion. The permissions restrictions don't make as much sense in the context of an internal GitHub Enterprise instance, but I don't want to assume that is true of everyone running Policy Bot.

   There might be other ways to solve this too; maybe you have to be a member of the organization to see user permissions or something. But I think that will still lead to to cases where you can see the policy but can't see who can approve.

2. Expanding teams, organizations, and permissions could significantly increase the number of API calls we make to GitHub. Right now, Policy Bot avoids listing membership (except when requesting reviewers) by testing if the specific users who left reviews are part of the necessary teams or have the right permissions. A naive implementation could end up listing the membership for all teams and organizations referenced by any rule in the policy.

   Limiting this only to "pending" rules helps a bit. Restricting the expansion only to rules using permissions also helps because that user list is the same for all rules in a given policy, so the requests only have to happen once per policy. We could also request this on-demand when a user expands the details for a rule, but that adds dynamic elements to a page that is currently static, so the implementation is more involved.

3. Some rules can accept approval by a lot of people (hundreds in the case of organizations.) Again, this is less of an issue for permission rules, which tend to have a smaller set of people, but we'll still have to consider how to format this information in the UI in a way that's useful when there's more than a few users.

4. Displaying all users with the necessary permissions is maybe not what people want, because the admin level includes organization owners. I think we'd probably want a heuristic like "only show show users who are members of teams explicitly granted permission on the repository." This matches the logic for reviewer assignment and would handle the common case for Palantir's internal policies, but could be confusing in other situations.

   A related consideration is if we should show users or teams in the permission case. At Palantir, teams are usually granted permissions and showing the team could point you to a team chat room instead of to individual people. My guess is teams usually prefer that people go to common channels for review requests, while PR authors might prefer specific people they can ask for review.

All of these are solvable problems and we also have the details section now, which wasn't the case originally. But hopefully this gives some insight into the implementation side of this requests. I'm also interested in hearing any other ideas to solve these problems (or arguments for why they're not actually problems!)