fix: GitHub mergeability bypassing apply

server/events/vcs/github_client.go

@@ -399,116 +399,92 @@ func (g *GithubClient) DiscardReviews(repo models.Repo, pull models.PullRequest)
 	return nil
 }
 
-// isRequiredCheck is a helper function to determine if a check is required or not
-func isRequiredCheck(check string, required []string) bool {
-	//in go1.18 can prob replace this with slices.Contains
-	for _, r := range required {
-		if r == check {
-			return true
-		}
+// IsMergeableMinusApply checks review decision (which takes into account CODEOWNERS) and required checks for PR (excluding the atlantis apply check).
+func (g *GithubClient) IsMergeableMinusApply(repo models.Repo, pull *github.PullRequest, vcsstatusname string) (bool, error) {
+	var query struct {
+		Repository struct {
+			PullRequest struct {
+				ReviewDecision githubv4.String
+				Commits        struct {
+					Nodes []struct {
+						Commit struct {
+							StatusCheckRollup struct {
+								Contexts struct {
+									PageInfo struct {
+										EndCursor   githubv4.String
+										HasNextPage githubv4.Boolean
+									}
+									Nodes []struct {
+										Typename githubv4.String `graphql:"__typename"`
+										CheckRun struct {
+											Name       githubv4.String
+											Conclusion githubv4.String
+											IsRequired githubv4.Boolean `graphql:"isRequired(pullRequestNumber: $number)"`
+										} `graphql:"... on CheckRun"`
+										StatusContext struct {
+											Context    githubv4.String
+											State      githubv4.String
+											IsRequired githubv4.Boolean `graphql:"isRequired(pullRequestNumber: $number)"`
+										} `graphql:"... on StatusContext"`
+									}
+								} `graphql:"contexts(first: 100, after: $cursor)"`
+							}
+						}
+					}
+				} `graphql:"commits(last: 1)"`
+			} `graphql:"pullRequest(number: $number)"`
+		} `graphql:"repository(owner: $owner, name: $name)"`
 	}
 
-	return false
-}
-
-// GetCombinedStatusMinusApply checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure.
-func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *github.PullRequest, vcstatusname string) (bool, error) {
-	//check combined status api
-	status, resp, err := g.client.Repositories.GetCombinedStatus(g.ctx, *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
-	if resp != nil {
-		g.logger.Debug("GET /repos/%v/%v/commits/%s/status returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, resp.StatusCode)
-	}
-	if err != nil {
-		return false, errors.Wrap(err, "getting combined status")
+	variables := map[string]interface{}{
+		"owner":  githubv4.String(repo.Owner),
+		"name":   githubv4.String(repo.Name),
+		"number": githubv4.Int(*pull.Number),
+		"cursor": (*githubv4.String)(nil),
 	}
 
-	//iterate over statuses - return false if we find one that isn't "apply" and doesn't have state = "success"
-	for _, r := range status.Statuses {
-		if strings.HasPrefix(*r.Context, fmt.Sprintf("%s/%s", vcstatusname, command.Apply.String())) {
-			continue
+	for {
+		err := g.v4Client.Query(g.ctx, &query, variables)
+
+		if err != nil {
+			return false, err
 		}
-		if *r.State != "success" {
+
+		if query.Repository.PullRequest.ReviewDecision != "APPROVED" && len(query.Repository.PullRequest.ReviewDecision) != 0 {
 			return false, nil
 		}
-	}
-
-	//get required status checks
-	required, resp, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref)
-	if resp != nil {
-		g.logger.Debug("GET /repos/%v/%v/branches/%s/protection returned: %v", repo.Owner, repo.Name, *pull.Base.Ref, resp.StatusCode)
-	}
-	if err != nil {
-		return false, errors.Wrap(err, "getting required status checks")
-	}
 
-	if required.RequiredStatusChecks == nil {
-		return true, nil
-	}
-
-	//check check suite/check run api
-	checksuites, resp, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil)
-	if resp != nil {
-		g.logger.Debug("GET /repos/%v/%v/commits/%s/check-suites returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, resp.StatusCode)
-	}
-	if err != nil {
-		return false, errors.Wrap(err, "getting check suites for ref")
-	}
-
-	//iterate over check completed check suites - return false if we find one that doesnt have conclusion = "success"
-	for _, c := range checksuites.CheckSuites {
-		if *c.Status == "completed" {
-			//iterate over the runs inside the suite
-			suite, resp, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, nil)
-			if resp != nil {
-				g.logger.Debug("GET /repos/%v/%v/check-suites/%d/check-runs returned: %v", *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, resp.StatusCode)
-			}
-			if err != nil {
-				return false, errors.Wrap(err, "getting check runs for check suite")
-			}
+		if len(query.Repository.PullRequest.Commits.Nodes) == 0 {
+			return false, errors.New("no commits found on PR")
+		}
 
-			for _, r := range suite.CheckRuns {
-				//check to see if the check is required
-				if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) {
-					if *c.Conclusion == "success" {
-						continue
-					}
+		for _, context := range query.Repository.PullRequest.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes {
+			switch context.Typename {
+			case "CheckRun":
+				if bool(context.CheckRun.IsRequired) &&
+					context.CheckRun.Conclusion != "SUCCESS" &&
+					!strings.HasPrefix(string(context.CheckRun.Name), fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) {
+					return false, nil
+				}
+			case "StatusContext":
+				if bool(context.StatusContext.IsRequired) &&
+					context.StatusContext.State != "SUCCESS" &&
+					!strings.HasPrefix(string(context.StatusContext.Context), fmt.Sprintf("%s/%s", vcsstatusname, command.Apply.String())) {
 					return false, nil
 				}
-				//ignore checks that arent required
-				continue
+			default:
+				return false, fmt.Errorf("unknown type of status check, %q", context.Typename)
 			}
 		}
-	}
-
-	return true, nil
-}
-
-// GetPullReviewDecision gets the pull review decision, which takes into account CODEOWNERS
-func (g *GithubClient) GetPullReviewDecision(repo models.Repo, pull models.PullRequest) (approvalStatus bool, err error) {
-	var query struct {
-		Repository struct {
-			PullRequest struct {
-				ReviewDecision string
-			} `graphql:"pullRequest(number: $number)"`
-		} `graphql:"repository(owner: $owner, name: $name)"`
-	}
-
-	variables := map[string]interface{}{
-		"owner":  githubv4.String(repo.Owner),
-		"name":   githubv4.String(repo.Name),
-		"number": githubv4.Int(pull.Num),
-	}
 
-	err = g.v4Client.Query(g.ctx, &query, variables)
-	if err != nil {
-		return approvalStatus, errors.Wrap(err, "getting reviewDecision")
-	}
+		if !query.Repository.PullRequest.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.PageInfo.HasNextPage {
+			break
+		}
 
-	if query.Repository.PullRequest.ReviewDecision == "APPROVED" || len(query.Repository.PullRequest.ReviewDecision) == 0 {
-		return true, nil
+		variables["cursor"] = githubv4.NewString(query.Repository.PullRequest.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.PageInfo.EndCursor)
 	}
 
-	return false, nil
+	return true, nil
 }
 
 // PullIsMergeable returns true if the pull request is mergeable.
@@ -518,7 +494,6 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
 		return false, errors.Wrap(err, "getting pull request")
 	}
 
-	state := githubPR.GetMergeableState()
 	// We map our mergeable check to when the GitHub merge button is clickable.
 	// This corresponds to the following states:
 	// clean: No conflicts, all requirements satisfied.
@@ -528,33 +503,22 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
 	// has_hooks: GitHub Enterprise only, if a repo has custom pre-receive
 	//            hooks. Merging is allowed (green box).
 	// See: https://github.com/octokit/octokit.net/issues/1763
-	if state != "clean" && state != "unstable" && state != "has_hooks" {
-		//mergeable bypass apply code hidden by feature flag
+	switch githubPR.GetMergeableState() {
+	case "clean", "unstable", "has_hooks":
+		return true, nil
+	case "blocked":
 		if g.config.AllowMergeableBypassApply {
 			g.logger.Debug("AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements")
-			if state == "blocked" {
-				//check status excluding atlantis apply
-				status, err := g.GetCombinedStatusMinusApply(repo, githubPR, vcsstatusname)
-				if err != nil {
-					return false, errors.Wrap(err, "getting pull request status")
-				}
-
-				//check to see if pr is approved using reviewDecision
-				approved, err := g.GetPullReviewDecision(repo, pull)
-				if err != nil {
-					return false, errors.Wrap(err, "getting pull request reviewDecision")
-				}
-
-				//if all other status checks EXCEPT atlantis/apply are successful, and the PR is approved based on reviewDecision, let it proceed
-				if status && approved {
-					return true, nil
-				}
+			isMergeableMinusApply, err := g.IsMergeableMinusApply(repo, githubPR, vcsstatusname)
+			if err != nil {
+				return false, errors.Wrap(err, "getting pull request status")
 			}
+			return isMergeableMinusApply, nil
 		}
-
+		return false, nil
+	default:
 		return false, nil
 	}
-	return true, nil
 }
 
 // GetPullRequest returns the pull request.