ampproject / amp-github-apps Goto Github PK

See ampproject/amphtml#20843

Overview

There are a handful of apps in this repository, each with different owners, deployment strategies, permissions, environments, etc. Most of this is documented, in one way or another, in the project subdirectories READMEs or code comments. The goal of this thread is to collect high-level info about each project (not full deployment instructions or READMEs). The below template contains questions which each app owner could answer to provide a useful starting point for looking at all the apps in one place and seeing where we might want to unify or standardize approaches:

Template

[App Name]

Owner(s): rcebulko, rsimha, ...
Directory: /my-app
Deployed on: [GCE | GAE | ???]
Project ID: amp-github-app-name
URL: [None | amp-app-link.appspot.com]
README link: README
Portability: [Can only work with amphtml | Could be modified to be used on other repos | is repository-agnostic/configurable]

Incoming requests:

• GitHub Webhooks
• Admin pages

Outgoing requests:

• GitHub API
• Travis API

Database: [Yes | No]
Authentication [No | GitHub OAuth | Basic Auth]
Custom lint config: [Yes | No]

Language/Framework: Probot
If Probot:

• Smee channel: http://smee.io/12345
• GitHub user: ampprojectbot
• GitHub App permissions (see https://github.com/settings/apps/my-app-name/permissions):
  • Checks: read & write (update/create check-runs on PRs)
  • Contents: read-only (read commits, branches, PRs, releases, etc)
  • Pull requests: read & write (read PR info, read & write comments)
• Webhooks:
  • Pull request: update check
  • Pull request review comment: unused
  • Pull request review: update check
  • Status: monitor status changes
  • Label: unused
• Personal access token name (from https://github.com/settings/tokens): ampprojectbot (owners bot, maybe other things)
• Required permissions [no idea if these are accurate, just examples]:
  • public_repo: Needed to read PR metadata, ...
  • read: org: Needed to list members of organization teams
  • repo: status: Needed to update check-runs, leave comments

Package management: [Yarn | npm | idk]
Testing framework: Jest
Test command: npm run test
Tests run on presubmit: [Yes | No]

Deployment workflow: [Manual | Automated]
Can this be run locally?: [Yes | No]
If so:

• Command to run locally: npm run dev
• Env. vars required to run locally:
  • GITHUB_ACCESS_TOKEN: Access token with at least public_repo permission
  • GITHUB_REPO_DIR: Path to local copy of ampproject/amphtml repository (ex. /tmp/amphtml)
  • GITHUB_REPO: Name of repository (ie. ampproject/amphtml or your fork)

Public environment (ie. config): [Shell script &| .env &| app.yaml &| GCE/GAE metadata &| deploy command arguments &| config file(s)]
Private environment (ie. secrets, tokens, keys): [Shell script &| .env &| app.yaml &| GCE/GAE metadata &| deploy command arguments &| config file(s)]
App uses a private-key.pem file for authentication: [Yes | No]

Plan

I'm going to start posting for the apps I've created/worked on, and then will start trying to do the same for the other apps with as much info as I can find in the repo.

/cc @ampproject/wg-infra let me know if you think of anything that might be missing. Ideally we can identify some pieces of this to standardize, if not with actual infrastructure then at least with more standard processes for things like keeping track of permissions, which apps use which tokens (since you can't see which token is which in the GitHub UI, making it risky to ever delete any), manage deployments, testing, etc.

/per @rsimha
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

Much like how we have always-notify and no-notify owners, we should support requiring a review. This could be useful for allowing certain groups or individuals to be "gatekeepers" of especially sensitive files or directories. It would also double as a solution to #499 , since the ampproject/reviewers team could be added as required at the top level.

The owners check should fail even if all files have normal owners coverage, but at least one file does not have approval from a required reviewer (or member of a required team). Ideally reviewer selection will prioritize required reviewers when making suggestions, but if not, after normal reviewer selection if there are still required reviewers missing it can add on randomly from the required sets.

when we find an owners file that is empty or invalid formatting, print a warning on the "checks" tab for it to be fixed.

Came up because some features aren't available on iOS Safari without a "secure context". In particular, it's not possible to demo PRs that have devicemotion listeners

/cc @kristoferbaxter

We should enable renovate for upgrade the packages in this repo to fix these and future security vulnerabilities.

Something like this already coded in https://github.com/ampproject/amphtml/blob/master/build-system/tasks/release-tagging.js, but it's not running consistently to my knowledge.

Here's how it'd work:

1. As soon as a release is created/change, a pull request appearing in the release's changelog gets tagged with the release number: via custom labels and/or comments. Ideally subscribers would get a notification "this pull request went into a release 12345".
2. When a release goes into canary or prod the associated pull requests get a label "in canary" or "in prod". Possibly also comments are added as well for notification.
3. The same tags/comments are extended to issues closed by an associated pull request. This is important, since most of people actually track issues and not pull requests.

/cc @rsimha

Currently GitHub apps are deployed manually using a local repository, which means a deployed app could be out of sync with what's in master.

cc/ @ampproject/wg-infra

Some contributors, such as top-level owners, may not want to be automatically notified or added as a reviewer to PRs. In other cases, some contributors may have horizontal ownership over a wide range of directories, such as over *.protoascii files. For these cases, we should support special syntax. Suggestion:

• ?someuser: The user someuser is an owner, but should not be auto-notified or added as a reviewer by the bot; they can be suggested by the bot, and added manually when the PR is ready for final approval
• !someuser: The user someuser is an owner, and even when their review is not required, they should always be notified/tagged/added as a reviewer so they can monitor changes
• !!someuser: The user someuser is a "super-owner" of the directory/file-pattern, and changes to the covered files may not be submitted without their approval

/cc @rsimha

Potential bug 1: the contributors badge count is different than the repo's contributor count

and

Potential bug 2: the cherry pick PR count seems too high. Also shouldn't we be counting issues, not PRs?

and
19 cherry pick issues since 6/27/2019

cc/ @ampproject/wg-infra

YAML is tricky to write and inconsistent to parse, which can cause problems as contributors try to write owners rules. Some things need to be quoted, some don't, lists and dicts can be defined in many ways, and it's generally less familiar to people than JSON.

Plan:

• Add support for parsing OWNERS.____; prefer the YAML form when present
• Create script to take the parsed form of an OWNERS.yaml file and output the matching OWNERS.____
• Generate & submit OWNERS.____ for all OWNERS.yaml files in amphtml
• Remove all OWNERS.yaml files
• Remove support for YAML

/cc @kristoferbaxter

Details in ampproject/amphtml#21275

/cc @ampproject/wg-runtime

There is an error with this repository's Renovate configuration that needs to be fixed. As a precaution, Renovate will stop PRs until it is resolved.

Error type: undefined. Note: this is a nested preset so please contact the preset author if you are unable to fix it yourself.

GitHub treats review comments as actual reviews. In #478 , the issue was that a post-approval comment was treated as a rejection, so we started ignoring comment reviews altogether. As shown in ampproject/amphtml#24853 , that leads to the issue where a reviewer can leave a comment review and they are then considered to have been "removed" from the reviewer set, resulting in the bot not seeing the potential owners coverage from that user/reviewer. The reviewer approval logic must be modified to view a comment as a rejection if it's the only review present, but should not be considered a rejection if the reviewer previously gave approval.

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.

scoped at directory level traversing down the subtree for ownership inheritance

@rsimha Any particular rules/linters we should include?

Per #499 , there is a need to have the owners check fail if a PR has approval from the OWNERS of the file, but not from someone in ampproject/reviewers-amphtml (or a comparable reviewer whitelist for other repositories). By integrating this into the owners check, we enable the repository to allow write-access to anyone.

The proposed solution is to allow a reviewerTeam property in the repository root OWNERS file, which can specify a GitHub team name. The owners check will report a failure for any PR unless at least one member of this team has reviewed it.

In any non-root owners file, this key will report an error and be ignored. If no such key is present in the repository root owners file, the condition will not be included.

Details at ampproject/amphtml#21815

The contributors badge count is different than the repo's contributor count

and

cc/ @ampproject/wg-infra

Currently the bot works by pushing to a cloud repository which is separate from this repo (since the project was started in its own repo), which I wrote a script to do by copy-pasting from a local amp-github-apps repo to a local amp-owners-bot repo, then pushing. When you run gcloud instances reset (or something like that), it restarts the instance and runs a startup-script which is part of the GCE instance metadata and contains a combination of startup script things (logging, checking out code, installing stuff, starting node under supervisord) along will a bunch of both sensitive and standard env variables. So, GCE settings and env vars are kinda one big blob at the moment. In theory there is a metadata key-value store in GCE settings, but I'm uncertain how easy it is to access these values within the app. The first step will be to move public-safe (ie. not Oauth/API tokens and keys) to a .env file where possible, and strip the massive startup-script into smaller pieces that can be checked into the repo. As that happens, we'll have a better idea of what options are available to us re: configuration.

This fragile deployment and configuration introduces a significant risk of bringing down the owners bot when trying to deploy updates or change configuration. It also present a significant obstacle to any other consumers of the owners bot app, since the core deployment and environment are not visible in the public repo.

/cc @rsimha @erwinmombay

this is because top level files will be constanty requested reviews from root level OWNERS. we need a way to make the top level files easier to be edited and approved.

The original implementation of the Owners bot was based around a Google Compute Engine (GCE) deployment. This decision was made because GCE provides local filesystem I/O, which is useful for maintaining a copy of the repository, using command-line-fu to identify owners files, and reading the contents of those owners files. No other files are read, and none are written.

Using GCE introduces a handful of issues, most notably:

• deployment is fragile, takes down the app, and relies on a startup script that is not checked in
• there is no reliable way to manage environment variables (especially sensitive keys/tokens); they are just embedded in the startup script, and can't be checked into the repository used to deploy
• the team is less familiar with GCE than GAE, making maintenance and debugging harder
• firewall restrictions around GCE make SSH and debugging difficult
• the amount of bootstrapping and configuration embedded only in GCE instance metadata makes it hard/impossible for other projects to make use of the owners bot

As a resolution to these issues, the plan is to migrate to Google App Engine (GAE). This will be a lot of work up-front, but should make automated and continuous deployment and development of the app much easier and less error-prone going forward.

Steps needed to migrate to a GAE deployment process:

• configure app to read from .env file; write the .env file within the startup script during migration
• hoist a base Repository class out of the LocalRepository class, and implement a RemoteRepository subclass
  • using the GitHub code search API, provide the listOwnersFiles method
  • using the GitHub file contents API, provide the readFile method
• maintain a map from owners file paths to the SHA of their latest commit
• implement a caching layer to prevent overloading GitHub API on file reads
• lift the collection and parsing of owners files/the ownership tree out of the owners check and into a single server-shared tree
• isolate bootstrapping code (ie. initializing teams and ownership trees)
• add webhooks to catch updates to owners files and update the corresponding rules in the cache
• create app.yaml and deploy to GAE
• migrate team and tree updating code to use Cron tasks
• spin down GCE version of app

/cc @rsimha @erwinmombay @danielrozenberg

I noticed that ampproject/amphtml#24730 state "needs approval" for a file despite the requester already having approved the PR. Might be because new commits were added?

Current Coverage
- [NEEDS APPROVAL] src/service/extensions-impl.js (requested: jridgewell)
- test/unit/test-extensions.js (choumx)

https://github.com/ampproject/amphtml/pull/24730/checks?check_run_id=236887665

Do not @mention an owner if they are already assigned as a reviewer.

Context: ampproject/amphtml#24709 (comment)

Today, we assign runtime bundle size increases for review to a randomly selected individual from a hard-coded set of reviewers.

I'm proposing that this be changed to the members of the wg-runtime team, who are more familiar with whether a core runtime size increase can be avoided.

/cc @ampproject/wg-runtime

When amp-owners-bot check is viewed more details about, there's the following link:

This currently leads to https://www.ampproject.org which is probably a placeholder. We should probably have it lead to a doc explaining OWNERS.

Since the owners bot is webhooked to run after new commits or review status changes, it triggers when someone removes a reviewer, causing the bot to immediately reassign the reviewer. This could also be an issue when commits are added later on, re-adding previously removed reviewers. The desired behavior is instead that reviewer assignment should only occur upon PR creation. This means that you couldn't trigger assignment after the fact, but that's a reasonable trade-off since the owners check will still show suggested reviewers if more are necessary.

Example: ampproject/amphtml#24388

For consistency, let's use the same .prettierrc as amphtml.

Eg, https://github.com/ampproject/amphtml/pull/23442/checks?check_run_id=173583540. The "resolve" link (https://amp-bundle-size-bot.appspot.com/) should allow us to force the build-size bot to recheck the GH approvals.

Today, failing checks result in a neutral status. These should be red X marks. (Unless there's a reason why this can't be done yet?)

The owners bot currently leaves a comment when a PR is created tagging any users with the always-notify modifier. When the PR is updated, it skips in order to avoid duplicate comments. Instead, it should update/edit (if possible via GitHub API) or delete and replace the existing comment.

/cc @estherkim @rsimha

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

Since some working groups contain owners without the ability to give approving reviews, it's possible people could be assigned as reviewers who can't actually provide the review/another reviewer would be needed. When suggesting reviewers, we should only suggest those in the ampproject/reviewers-amphtml group.

One concern to mind when implementing this is that this is a relatively amphtml-centric need, but the solution should be relatively repository-agnostic so that other repos utilizing the owners bot aren't adversely affected.

This shouldn't have any effect on ownership coverage; it should only filter out which users can be considered during the reviewer selection algorithm. Proposed approach:

• Provide a isAllowedReviewer(username) --> boolean test function to the reviewer selection algorithm and filter when selecting reviewers
• Let isAllowedReviewer default to username => true
• Add a ALLOWED_REVIEWERS_TEAM env variable where a team can be supplied
• When process.env.ALLOWED_REVIEWERS_TEAM is present in the team list, provide username => team.allUsernames.includes(username) as the isAllowedReviewer test function (or some set-based variant)

@rsimha thoughts?

Context: #443 (comment)

Disabled in #103

See title :)

Seen in the bundle size bot and test-status bot, Travis VM IP addresses are hardcoded to determine that an API request is coming from Travis, although in practice, IP addresses are subject to change and are out of our control.

I've seen some instances in Travis where the job fails because the IP address falls outside the accepted range. Example here - https://travis-ci.org/ampproject/amphtml/jobs/550468070

My suggestion is to use a service account or another authentication method that doesn't rely on IP addresses.

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).

As can be seen in PRs listed below, the owners bot is not requesting reviews for package.json and yarn.lock dependency changes. It does appear to recognize approving reviews, and the owners check itself seems to be functioning normally.

I created ampproject/amphtml#25193 to test assignment of root level files, and it requested a review from mrjoro as expected. This suggests the issue is likely not with the owners bot logic or review requesting, but instead is likely to be a quirk of how the GitHub API handles bot PRs and what the event payloads look like for the corresponding webhooks.

ampproject/amphtml#25193
ampproject/amphtml#23726
ampproject/amphtml#25036

/cc @rsimha

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

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).