Skip to content

Expand pr bot to python #21791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tvalentyn merged 8 commits into from
Jun 17, 2022
Merged

Expand pr bot to python #21791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a128452
Expand pr bot to python
damccorm Jun 10, 2022
81d5fdf
Push reviewer changes
damccorm Jun 10, 2022
4f30a00
Limit to Python WG + Brian
damccorm Jun 13, 2022
ed0cb2e
Add Pablo
damccorm Jun 14, 2022
34e5a31
Add Yichi
damccorm Jun 15, 2022
7c528ef
Set start date and format
damccorm Jun 16, 2022
7b830d2
Merge branch 'users/damccorm/prbot-python' of https://github.com/damc…
damccorm Jun 16, 2022
57aa3ab
Feedback
damccorm Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/REVIEWERS.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,14 @@ labels:
- youngoli
- riteshghorse
exclusionList: []
- name: Python
reviewers:
- AnandInguva
- yeandy
- TheNeuralBit
- ryanthompson591
- tvalentyn
- pabloem
- y1chi
exclusionList: []
fallbackReviewers: []
19 changes: 14 additions & 5 deletions scripts/ci/pr-bot/processNewPrs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,30 @@ import { CheckStatus } from "./shared/checks";
* 4) Are closed
* 5) Have already been processed
* 6) Have notifications stopped
* 7) The pr doesn't contain the go label (temporary). TODO(damccorm) - remove this when we're ready to roll this out to everyone.
* 7) The pr doesn't contain the go or python labels (temporary). TODO(damccorm) - remove this when we're ready to roll this out to everyone.
* 8) The pr happens after the date we turn on the automation. TODO(damccorm) - remove this once this has been rolled out for a while.
* unless we're supposed to remind the user after tests pass
* (in which case that's all we need to do).
*/
function needsProcessed(pull: any, prState: typeof Pr): boolean {
if (!pull.labels.find((label) => label.name.toLowerCase() === "go")) {
if (
Copy link
Contributor

@tvalentyn tvalentyn Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re item 6) above : for my education, what does it mean to have notifications stopped?
item 7 is out of sync now.

Copy link
Contributor Author

@damccorm damccorm Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anytime a reviewer manually requests review (e.g. R: @user), the bot turns itself off (and comments that its doing so). For example - #21852 (comment)

Fixed the comment

!pull.labels.find(
(label) =>
label.name.toLowerCase() === "go" ||
label.name.toLowerCase() === "python"
)
) {
console.log(
`Skipping PR ${pull.number} because it doesn't contain the go label`
`Skipping PR ${pull.number} because it doesn't contain the go or python labels`
);
return false;
}
const firstPrToProcess = new Date(2022, 2, 2, 20);
const firstPrToProcess = new Date(2022, 5, 16, 14); // June 16 2022, 14:00 UTC (note that Java months are 0 indexed)
const createdAt = new Date(pull.created_at);
if (createdAt < firstPrToProcess) {
if (
createdAt < firstPrToProcess &&
!pull.labels.find((label) => label.name.toLowerCase() === "go")
Copy link
Contributor

@tvalentyn tvalentyn Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to exclude go here given that the bot has already been enabled for go? and probably all outstanding go prs have been processed already, except for maybe a couple go of PRs that happen to be submitted before two successive iteration of this script?

Copy link
Contributor Author

@damccorm damccorm Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're excluding go because we don't want it to be skipped ever. Its worth noting that this would retroactively apply to in flight prs, even if the PR bot has already taken some action on them, so without this exclusion this would break the flow on all in-flight PRs. Functionally, the biggest implication would be that if its been assigned to a non-committer and that non-committer approves it won't get auto-routed to a committer

) {
console.log(
`Skipping PR ${pull.number} because it was created at ${createdAt}, before the first pr to process date of ${firstPrToProcess}`
);
Expand Down
10 changes: 8 additions & 2 deletions scripts/ci/pr-bot/processPrUpdate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,14 @@ async function processPrUpdate() {

// TODO(damccorm) - remove this when we roll out to more than go
const existingLabels = payload.issue?.labels || payload.pull_request?.labels;
if (!existingLabels.find((label) => label.name.toLowerCase() === "go")) {
console.log("Does not contain the go label - skipping");
if (
!existingLabels.find(
(label) =>
label.name.toLowerCase() === "go" ||
label.name.toLowerCase() === "python"
)
) {
console.log("Does not contain the go or python labels - skipping");
Copy link
Contributor

@tvalentyn tvalentyn Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my education, where can these logs be inspected?

Copy link
Contributor Author

@damccorm damccorm Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actions console for the run - https://github.com/apache/beam/actions/workflows/pr-bot-new-prs.yml

return;
}

Expand Down