Update the reviewer bot to notify based on draft reopen time in addition to event time #12428
Conversation
…ion to event time
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
| if earliestChangesRequested != nil { | ||
| return waitingForContributor, *earliestChangesRequested.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestChangesRequested.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } | ||
| if earliestApproved != nil { | ||
| return waitingForMerge, *earliestApproved.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestApproved.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForMerge, timeState, nil | ||
| } | ||
| if earliestCommented != nil { | ||
| return waitingForContributor, *earliestCommented.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestCommented.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } |
There was a problem hiding this comment.
I think the logic here isn't quite right - for example, earliestChangesRequested could end up holding a change request from before the PR was reopened, which would cause the func to return early - potentially missing an earliestApproved that comes after the PR was reopened.
I think that the reviews need additional filtering to skip these events instead.
There was a problem hiding this comment.
We only consider the most recent review from a core reviewer. See the case:switch on line 340.
There was a problem hiding this comment.
The behavior I described does theoretically exist - here's a test case that demonstrates it:
"changes_requested before ready_for_review with approved after": {
pullRequest: &github.PullRequest{
User: &github.User{Login: github.String("author")},
CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)},
},
issueEvents: []*github.IssueEvent{
&github.IssueEvent{
Event: github.String("review_requested"),
CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)},
RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)},
},
&github.IssueEvent{
Event: github.String("ready_for_review"),
CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, // Earlier ready
},
},
reviews: []*github.PullRequestReview{
&github.PullRequestReview{
User: &github.User{Login: github.String(firstCoreReviewer)},
State: github.String("CHANGES_REQUESTED"),
SubmittedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)},
},
&github.PullRequestReview{
User: &github.User{Login: github.String(secondCoreReviewer)},
State: github.String("APPROVED"),
SubmittedAt: &github.Timestamp{time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC)},
},
},
expectState: waitingForMerge,
expectSince: time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC),
},However, after thinking it over, I think the behavior you have here makes sense... if a change was requested, and then the PR was closed, left for a long time, and then reopened (somehow without a review being requested) we'd want to keep it in "Changes requested" but just reset the clock. (But actually ready_for_review requests a new review, so the only case that can actually happen here is that a review was requested and we reset the clock for when the review should have started.)
There was a problem hiding this comment.
hmm I think there actually is an issue here with dismissed reviews not clearing an earlier changes_requested.
Theoretically the new reviewer should dismiss the old reviewer if their feedback is no-longer applicable and/or they can no longer approve.
There was a problem hiding this comment.
Is there an issue with dismissed reviews that was not already present? I don't remember exactly why I implemented this as is; I'm happy to change the behavior around dismissed reviews but I'd rather hold it for a separate PR unless there's a specific impact to dismissed reviews from this work, since it may have additional implications.
| if earliestChangesRequested != nil { | ||
| return waitingForContributor, *earliestChangesRequested.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestChangesRequested.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } | ||
| if earliestApproved != nil { | ||
| return waitingForMerge, *earliestApproved.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestApproved.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForMerge, timeState, nil | ||
| } | ||
| if earliestCommented != nil { | ||
| return waitingForContributor, *earliestCommented.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestCommented.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } |
There was a problem hiding this comment.
The behavior I described does theoretically exist - here's a test case that demonstrates it:
"changes_requested before ready_for_review with approved after": {
pullRequest: &github.PullRequest{
User: &github.User{Login: github.String("author")},
CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)},
},
issueEvents: []*github.IssueEvent{
&github.IssueEvent{
Event: github.String("review_requested"),
CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)},
RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)},
},
&github.IssueEvent{
Event: github.String("ready_for_review"),
CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, // Earlier ready
},
},
reviews: []*github.PullRequestReview{
&github.PullRequestReview{
User: &github.User{Login: github.String(firstCoreReviewer)},
State: github.String("CHANGES_REQUESTED"),
SubmittedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)},
},
&github.PullRequestReview{
User: &github.User{Login: github.String(secondCoreReviewer)},
State: github.String("APPROVED"),
SubmittedAt: &github.Timestamp{time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC)},
},
},
expectState: waitingForMerge,
expectSince: time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC),
},However, after thinking it over, I think the behavior you have here makes sense... if a change was requested, and then the PR was closed, left for a long time, and then reopened (somehow without a review being requested) we'd want to keep it in "Changes requested" but just reset the clock. (But actually ready_for_review requests a new review, so the only case that can actually happen here is that a review was requested and we reset the clock for when the review should have started.)
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
| if earliestChangesRequested != nil { | ||
| return waitingForContributor, *earliestChangesRequested.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestChangesRequested.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } | ||
| if earliestApproved != nil { | ||
| return waitingForMerge, *earliestApproved.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestApproved.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForMerge, timeState, nil | ||
| } | ||
| if earliestCommented != nil { | ||
| return waitingForContributor, *earliestCommented.SubmittedAt.GetTime(), nil | ||
| timeState := maxTime(*earliestCommented.SubmittedAt.GetTime(), readyForReviewTime) | ||
| return waitingForContributor, timeState, nil | ||
| } |
There was a problem hiding this comment.
Is there an issue with dismissed reviews that was not already present? I don't remember exactly why I implemented this as is; I'm happy to change the behavior around dismissed reviews but I'd rather hold it for a separate PR unless there's a specific impact to dismissed reviews from this work, since it may have additional implications.
closes hashicorp/terraform-provider-google#20142
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.