From 04f20ddb2bf6b33abd4fe468abec0e02d99d40d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 15 Nov 2019 11:43:00 +0100 Subject: [PATCH 1/3] Use `statusCheckRollup` internal GraphQL to simplify Statuses+Checks With the old approach, we had to enumerate all StatusContexts and CheckRuns to calculate whether a PR has passing or failing CI status. Using `statusCheckRollup` which is behind the `pe_mobile` feature flag, this is somewhat simpler because both StatusContexts and CheckRuns are now behind the same connection. Additionally, should we decide to *not* show the number of passing/failing checks, this now approach allows us to consume the `statusCheckRollup { state }` field and avoid paginated collections and calculating the "winning" state altogether. --- api/queries.go | 61 ++++++++++++++++++------------------------------- command/root.go | 1 + 2 files changed, 23 insertions(+), 39 deletions(-) diff --git a/api/queries.go b/api/queries.go index 4f1f74ed5..d18b2c679 100644 --- a/api/queries.go +++ b/api/queries.go @@ -35,17 +35,11 @@ type PullRequest struct { Commits struct { Nodes []struct { Commit struct { - Status struct { - Contexts []struct { - State string - } - } - CheckSuites struct { - Nodes []struct { - CheckRuns struct { - Nodes []struct { - Conclusion string - } + StatusCheckRollup struct { + Contexts struct { + Nodes []struct { + State string + Conclusion string } } } @@ -97,32 +91,23 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { return } commit := pr.Commits.Nodes[0].Commit - for _, status := range commit.Status.Contexts { - switch status.State { - case "SUCCESS": + for _, c := range commit.StatusCheckRollup.Contexts.Nodes { + state := c.State + if state == "" { + state = c.Conclusion + } + switch state { + case "SUCCESS", "NEUTRAL", "SKIPPED": summary.Passing++ - case "EXPECTED", "ERROR", "FAILURE": + case "ERROR", "FAILURE", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED": summary.Failing++ - case "PENDING": + case "EXPECTED", "QUEUED", "PENDING", "IN_PROGRESS": summary.Pending++ default: - panic(fmt.Errorf("unsupported status: %q", status.State)) + panic(fmt.Errorf("unsupported status: %q", state)) } summary.Total++ } - for _, checkSuite := range commit.CheckSuites.Nodes { - for _, checkRun := range checkSuite.CheckRuns.Nodes { - switch checkRun.Conclusion { - case "SUCCESS", "NEUTRAL": - summary.Passing++ - case "FAILURE", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED": - summary.Failing++ - default: - panic(fmt.Errorf("unsupported check conclusion: %q", checkRun.Conclusion)) - } - summary.Total++ - } - } return } @@ -267,15 +252,13 @@ func PullRequests(client *Client, ghRepo Repo, currentBranch, currentUsername st commits(last: 1) { nodes { commit { - status { - contexts { - state - } - } - checkSuites(first: 50) { - nodes { - checkRuns(first: 50) { - nodes { + statusCheckRollup { + contexts(last: 100) { + nodes { + ...on StatusContext { + state + } + ...on CheckRun { conclusion } } diff --git a/command/root.go b/command/root.go index 22c0122a1..4fe921391 100644 --- a/command/root.go +++ b/command/root.go @@ -75,6 +75,7 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { // antiope-preview: Checks // shadow-cat-preview: Draft pull requests api.AddHeader("Accept", "application/vnd.github.antiope-preview+json, application/vnd.github.shadow-cat-preview"), + api.AddHeader("GraphQL-Features", "pe_mobile"), } if verbose := os.Getenv("DEBUG"); verbose != "" { opts = append(opts, api.VerboseLog(os.Stderr)) From 6e894a0eabec0f683e2794b961374b8b7f3c65d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 15 Nov 2019 12:00:13 +0100 Subject: [PATCH 2/3] Use `reviewDecision` internal GraphQL to simplify review state Before, we've used the `reviews` connection to iterate through all reviews chronologically and try to guess the final state of reviews. This approach had several problems: - it didn't handle dismissed reviews well, - the conclusion would likely be wrong if the number of total reviews exceeded the per-page limit. The `pe_mobile` feature flag exposes the `reviewDecision` field that handles all of this for us. --- api/queries.go | 36 +++++++----------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/api/queries.go b/api/queries.go index d18b2c679..c4bfa8353 100644 --- a/api/queries.go +++ b/api/queries.go @@ -23,14 +23,7 @@ type PullRequest struct { Login string } - Reviews struct { - Nodes []struct { - State string - Author struct { - Login string - } - } - } + ReviewDecision string Commits struct { Nodes []struct { @@ -62,19 +55,11 @@ type PullRequestReviewStatus struct { func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { status := PullRequestReviewStatus{} - reviewMap := map[string]string{} - // Reviews will include every review on record, including consecutive ones - // from the same actor. Consolidate them into latest state per reviewer. - for _, review := range pr.Reviews.Nodes { - reviewMap[review.Author.Login] = review.State - } - for _, state := range reviewMap { - switch state { - case "CHANGES_REQUESTED": - status.ChangesRequested = true - case "APPROVED": - status.Approved = true - } + switch pr.ReviewDecision { + case "CHANGES_REQUESTED": + status.ChangesRequested = true + case "APPROVED": + status.Approved = true } return status } @@ -270,14 +255,7 @@ func PullRequests(client *Client, ghRepo Repo, currentBranch, currentUsername st } fragment prWithReviews on PullRequest { ...pr - reviews(last: 20) { - nodes { - state - author { - login - } - } - } + reviewDecision } query($owner: String!, $repo: String!, $headRefName: String!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) { From be55f81e16992df46bd037b1aece6d4e4e483e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 15 Nov 2019 12:05:05 +0100 Subject: [PATCH 3/3] Add "review required" notice to PRs if applicable --- api/queries.go | 3 +++ command/pr.go | 2 ++ 2 files changed, 5 insertions(+) diff --git a/api/queries.go b/api/queries.go index c4bfa8353..2e14fb26b 100644 --- a/api/queries.go +++ b/api/queries.go @@ -51,6 +51,7 @@ func (pr PullRequest) HeadLabel() string { type PullRequestReviewStatus struct { ChangesRequested bool Approved bool + ReviewRequired bool } func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { @@ -60,6 +61,8 @@ func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { status.ChangesRequested = true case "APPROVED": status.Approved = true + case "REVIEW_REQUIRED": + status.ReviewRequired = true } return status } diff --git a/command/pr.go b/command/pr.go index 0445b81d6..eba61704d 100644 --- a/command/pr.go +++ b/command/pr.go @@ -275,6 +275,8 @@ func printPrs(prs ...api.PullRequest) { if reviews.ChangesRequested { fmt.Printf(" - %s", utils.Red("changes requested")) + } else if reviews.ReviewRequired { + fmt.Printf(" - %s", utils.Yellow("review required")) } else if reviews.Approved { fmt.Printf(" - %s", utils.Green("approved")) }