Add support for triage and maintain permissions #294
Conversation
Deprecate the "admins" and "write_collaborators" properties and replace them with a new "permissions" key that accepts all permission levels. Like before, users must specify levels explicitly and there is no permission inheritance (e.g. users with admin permissions do not count as having write permissions.) The GitHub REST API does not expose the new permissions, so switch collaborator lookup to GraphQL, which does include them. This will remove caching of collaborator information, so we may need to revisit that to reduce request rates.
This will always operate on the current pull request/repository and can share cached data with RepositoryCollaborators() so there's no reason to make it part of the membership/cross-org context.
| # A user must have a permission in this list for their approval to count for | ||
| # this rule. Valid permissions are "admin", "maintain", "write", "triage", | ||
| # and "read". | ||
| permissions: ["admin", "maintain", "write"] |
Rename RepositoryPermission to Permission and use it directly in the YAML configuration. This avoids parsing later on and adds validation, since the current validation logic really only checks that the YAML parses cleanly.
This required quite a bit of API fiddling because the APIs here are pretty inconsistent. I settled on a heuristic approach, but an alternate method would be to make more API calls so that we: 1. List team permissions and then expand membership 2. List direct repository collaborators 3. Anything else must come from the org
This requires fixing up some tests which happened to work before because write_collaborators did not work for teams. Also remove a test that was only testing the mock pull.Context implementation.
Use additional API requests (probably 1 in practice) to get this information more directly, instead of using heuristics based on API misbehavior.
Only include results relevant to situation being tested to minimize breaks when unrelated behavior changes.
|
Still need to do some more local testing, but this should be ready for review now. This ended up including a bit of refactoring for reviewer assignment that wasn't strictly necessary for the new permission types, but should make things more efficient when using |
Started refactoring this in a previous commit and then never finished.
| return nil | ||
| } | ||
|
|
||
| func selectUserReviewers(ctx context.Context, prctx pull.Context, selection *Selection, result *common.Result, r *rand.Rand) error { |
There was a problem hiding this comment.
this function feels a lot more readable now, thanks for fixing it up.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package pull |
There was a problem hiding this comment.
I wonder if it would be nice to put this in its own package (instead of pull), maybe it would be useful in octo-correct?
There was a problem hiding this comment.
Given this is a pretty simple type and these two systems don't directly interact, I prefer copying this (or something similar) into other apps that need it rather than introducing a new dependency package or moving it into a common dependency like go-githubapp (where I feel it doesn't really fit with any existing functionality.)
There was a problem hiding this comment.
I was thinking maybe a package in policy-bot, but I think you're right.
I mistakenly assumed the "query" parameter was an exact match on user name, but it's actually a substring filter. This means we need to iterate over returned collaborators to find the one we want.
permissionsSources is apparently not available to GitHub Apps, regardless of the app's permissions.
|
Ok, this should be set for final review now. Local testing looks good and I think I found all the API scorpions 🦂. |
|
👍 Lets try it out! |
Deprecate the
adminsandwrite_collaboratorsproperties and replace them with a newpermissionskey that accepts all permission levels. Like before, users must specify levels explicitly and there is no permission inheritance (e.g. users with admin permissions do not count as having write permissions.)The GitHub REST API does not expose the new permissions, so switch collaborator lookup to GraphQL, which does include them. This will remove caching of collaborator information, so we may need to revisit that to reduce request rates.
I still need to fix up reviewer assignment to work with the new fields. That's coming in a future commit.