[BEAM-13925] Add most of the supporting files for the pr management automation #16898
Conversation
jrmccluskey
left a comment
jrmccluskey
left a comment
There was a problem hiding this comment.
I'll leave a TypeScript-focused review for someone else, but I have a rough idea of the structure here. Looks like a good system, and I'm happy to be involved in the reviewer pool for the first run at this!
|
If he has time @KonradJanica would be a good reviewer for typescript. |
| export const REPO = "beam"; | ||
| export const PATH_TO_CONFIG_FILE = path.join( | ||
| __dirname, | ||
| "../../../../../.github/REVIEWERS.yml" |
There was a problem hiding this comment.
This is very brittle and difficult to read. Could we use a path from the root instead?
There was a problem hiding this comment.
I didn't want to do that since I don't think there's a consistent way to get the repo root from both GitHub actions and locally, and I'd like this constant to be valid in both cases.
| const { REPO_OWNER, REPO } = require("./constants"); | ||
|
|
||
| export function getGitHubClient() { | ||
| let githubToken = process.env["GITHUB_TOKEN"]; |
There was a problem hiding this comment.
Preferably this token could be stored as a constant somewhere after the env has initialized.
There was a problem hiding this comment.
I'd actually vote to keep it as is. The env check is cheap, and this is the only place we ever access it. Not storing it in any persistent state has the advantage of making it very obvious that we're not accidentally leaking it anywhere.
|
|
||
| export async function nextActionReviewers( | ||
| pullNumber: number, | ||
| existingLabels: any[] |
There was a problem hiding this comment.
Avoid any type. Add a definition to the top of the file if needed:
interface Label {
name: string;
}
There was a problem hiding this comment.
Done - I think there are places in this work where it won't be beneficial to avoid any though (specifically dealing with GitHub payloads which aren't typed on their own). This one is probably fine though.
| * limitations under the License. | ||
| */ | ||
|
|
||
| export function allChecksPassed(reviewersToNotify: string[]): string { |
There was a problem hiding this comment.
Exported functions typically must have comments, most linters will complain about this. That being said, I'm not sure how strict we should be on this code.
There was a problem hiding this comment.
None of this is being published to a package (its all internal code), so I'd vote we remain pretty loose with that. There's functionally not a difference between exported and non-exported files other than maintaining good repo structure.
scripts/ci/pr-bot/shared/pr.ts
Outdated
| for (let i = 0; i < labels.length; i++) { | ||
| let label = labels[i]; | ||
| if ( | ||
| await github.checkIfCommitter(this.reviewersAssignedForLabels[label]) |
There was a problem hiding this comment.
This call isn't batched so it will be very slow. Is there a batched version of the API?
There was a problem hiding this comment.
There isn't - I would expect relatively few calls here though - this represents all reviewers assigned to the pr, so there should never be more than a few (definitely <=5, more likely 1-2).
|
R: @lostluck - not sure if there's a better committer, so I'm roping you in (feel free to redirect). Would you mind taking this over the finish line, Jack and Konrad (TS reviewer) have already approved |
5 participants
Right now, we don't have a well defined automated system for staying on top of pull request reviews - we rely on contributors being able to find the correct OWNERS file and committers manually triaging/calling attention to old pull requests. This is the start of an effort to improve our automation around this. Design doc for the full effort here
I have the code to handle new prs and pr updates in a repo here - https://github.com/damccorm/beam-pr-bot-demo - this is a subset of the full set of changes split up to try to keep reviews manageable. This Pr introduces most of the shared files used by the automations for things like interacting with GitHub, storing/interacting with our durable state on a dedicated branch and configuration, and some constants used by the automations.
Initially, I am filtering all bot activity prs with the Go label. This is to minimize the blast radius of the automation in case it doesn't work perfectly out the gate or we find it unhelpful. Eventually, I plan on expanding to the rest of the repo.
You can see see the completed portions of the bot in action in this pr and this pr (they show different use cases and exercise almost all of the behavior of the bot).
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.