we're probably reaching out to actual github api's right now to make some calls. need to figure out which ones and mock them out

this is high priority

currently github groups are not identified and expanded into the full list to be evaluated so they are currently ignored

currently the only supported event is a check_run rerequested and I forgot to support check_suite rerequested

For example, package.json receives frequent updates from renovate-bot. Often times, the build cop needs to ask others for a review based on the actual dependency being upgraded (the line being changed).

I wonder if the owners bot can eventually figure this out and intelligently assign a reviewer based on which dependency (line) is upgraded (changed).

assuming only 1 file changed in a 3 level deep directory:

original algorithm was very simple:
look for the closest OWNER file for a file changed. The problem with this was that if you were a root owner and the file changed was in a directory 3 levels deep you would be given the owners list of that directory and not be approved right away.

current algorithm:
look for ALL the OWNER files starting from the current directory of the file and its ancestor up to the root accumulating all usernames as possible approvers. This solves the problem of the original algorithm but also is very verbose in its owner listing (even with removing dupes etc).

possible future algorithm:
look for the closest OWNER for a file changed and choose that as a candidate (possibly to post as a comment) but ALSO traverse the ancestor hierarchy in case the current PR author is an ancestor owner and approve the PR right away.

/to @cramforce for comments and suggestions

scoped at dir level traversing down

sometimes there is a single owner of a directory who creates a pull request. who then becomes the reviewer? It should traverse up the owners tree

this is so that changes to the sort order in the OWNERS.yaml file doesnt affect us detecting that its still just the same list

currently we can't even run the api tests on travis because we run exec on the file system and we need a target git repo beside the source code to read owners.yaml files from

... so that this code can live next to bundle-size and test-status at https://github.com/ampproject/amp-github-apps

/cc @cramforce @mrjoro

Currently we work off of the "APPROVED" label but once github exposes the usernames of the approvers of a PR we can switch to that to change the status

I pinged github about this a few months ago and they said it was coming up but 🤷‍♂️ its been a while

For AMP this might be an OK link
https://github.com/ampproject/amphtml/blob/master/GOVERNANCE.md#owners

But we should probably have a dedicated doc in https://github.com/ampproject/amphtml/tree/master/contributing

CC @mrjoro

We shouldn't always @-mention the whole team :)

We should probably:

• randomly select one reviewer
• and maybe keep the rest of the list, but remove the @ from the non-selected reviewers

Incoming renovate PRs tend to get ignored by most build cops. The owners bot is in a good position to assign each incoming PR to the current build cop for prompt attention.

We can do two things when a renovate PR is initially created, or a new commit is pushed:

• Assign (or reassign) the PR to the current build cop
• Add the current build cop as reviewer

/cc @danielrozenberg who is working on an API to return the github ID of the current build cop
/cc @mrjoro

processing might take longer than expected so we need to create a queued check before submitting the final status of the check

Trigger criteria:

• Author is not from AMP team
• All changed files are under ads/

Actions:

• Put the PR to project "3P Ads & Analytics Support".
• If possible, move the PR to the "backlog" column of the project.
• Make a comment:

To accelerate the review process, please make sure you read the guidelines and test the ad locally before sending out the PR. @lannka

this just means we will be no worse than the current process since a github approval is still required. but with a massive number of required reviewers it will become cumbersome to require all of them to approve.

That way people can quickly find alternative approvers.

See e.g. ampproject/amphtml#8175

I do not need any extra approvals. In that case the bot should not comment.

currently the bot does not re-evaluate

When someone new to the codebase wants to edit an OWNERS.yaml file, it's hard to find the format for declaring owners for individual files / directories, and for declaring a working group as an owner.

A single line comment that points to the OWNERS.yaml spec would be useful.

I'm happy to write up a PR if a link exists.

/cc @mrjoro

For example, package.json gets updated very frequently by the renovate bot. A build cop should be able to approve and merge those PRs without needing a review from the individual owners of the root directory (or that file).

/cc @mrjoro @cramforce

For the validator, we sync changes developed in Google's internal repo up into Github (we also sync the other direction sometimes, but that's not affected here). This is such a change:
ampproject/amphtml#8134

Ampprojectbot assignes the .protoascii (validator specification files) and .out files (validator test files) to owners of the extensions. This means that getting such a sync requires many approvals, likely slowing down our process.

The solution is probably to patternmatch the protoascii and html and out files for the validator so that we can commit them even if they're in some extension directory. Thanks much for considering.

auto mass assigning reviewers to a pull request doesnt seem like a good idea especially on a PR that touches a large number of files with different and multiple OWNERS usernames attached to them. since there are multiple "candidates" most reviewers will think that since there are other reviewers assigned that they do not need to review the PR

It's confusing to have a non-blocking check that applies a red x mark, because it makes people believe they can't merge when they actually can.