fix: do not apply approved label if change requested

An outstanding CHANGES_REQUESTED state prevents a PR from being merged even if another reviewer approves the PR. This change makes it so that the approved label cannot be added until all change requested states have been resolved. This prevents a scenario where tide will repeatedly try to merge a PR and be rejected by GitHub due to the changes requested state.
kubernetes-sigs · Sep 5, 2024 · 1bdbd87 · 1bdbd87
1 parent b4c04a9
commit 1bdbd87
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 17 deletions.
diff --git a/pkg/plugins/approve/approve.go b/pkg/plugins/approve/approve.go
@@ -590,15 +590,21 @@ func addApprovers(approversHandler *approvers.Approvers, approveComments []*comm
 			continue
 		}
 
-		if reviewActsAsApprove && c.ReviewState == github.ReviewStateApproved {
-			approversHandler.AddApprover(
-				c.Author,
-				c.HTMLURL,
-				false,
-			)
+		if c.ReviewState == github.ReviewStateApproved {
+			approversHandler.RemoveChangeRequested(c.Author)
+			if reviewActsAsApprove {
+				approversHandler.AddApprover(
+					c.Author,
+					c.HTMLURL,
+					false,
+				)
+			}
 		}
-		if reviewActsAsApprove && c.ReviewState == github.ReviewStateChangesRequested {
-			approversHandler.RemoveApprover(c.Author)
+		if c.ReviewState == github.ReviewStateChangesRequested {
+			approversHandler.AddChangeRequested(c.Author)
+			if reviewActsAsApprove {
+				approversHandler.RemoveApprover(c.Author)
+			}
 		}
 
 		for _, match := range commandRegex.FindAllStringSubmatch(c.Body, -1) {

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
@@ -1054,11 +1054,14 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 <!-- META={"approvers":["alice","cjwagner"]} -->`,
 		},
 		{
-			name:                "approve command supersedes simultaneous changes requested review",
-			hasLabel:            false,
-			files:               []string{"a/a.go"},
-			comments:            []github.IssueComment{},
-			reviews:             []github.Review{newTestReview("Alice", "/approve", github.ReviewStateChangesRequested)},
+			name:     "approve command supersedes prior changes requested review",
+			hasLabel: false,
+			files:    []string{"a/a.go"},
+			comments: []github.IssueComment{},
+			reviews: []github.Review{
+				newTestReview("Alice", "/approve", github.ReviewStateChangesRequested),
+				newTestReview("Alice", "/approve", github.ReviewStateApproved),
+			},
 			selfApprove:         false,
 			needsIssue:          false,
 			lgtmActsAsApprove:   false,
@@ -1081,6 +1084,41 @@ Needs approval from an approver in each of these files:
 
 - ~~[a/OWNERS](https://github.com/org/repo/blob/master/a/OWNERS)~~ [Alice]
 
+Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
+</details>
+<!-- META={"approvers":[]} -->`,
+		},
+		{
+			name:     "changes requested supersedes approve command",
+			hasLabel: false,
+			files:    []string{"a/a.go"},
+			comments: []github.IssueComment{},
+			reviews: []github.Review{
+				newTestReview("Alice", "/approve", github.ReviewStateChangesRequested),
+			},
+			selfApprove:         false,
+			needsIssue:          false,
+			lgtmActsAsApprove:   false,
+			reviewActsAsApprove: true,
+			githubLinkURL:       &url.URL{Scheme: "https", Host: "github.com"},
+
+			expectDelete:  false,
+			expectToggle:  false,
+			expectComment: true,
+			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
+
+This pull-request has been approved by: *<a href="" title="Approved">Alice</a>*
+
+The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=org%2Frepo).
+
+The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)
+
+<details >
+Needs approval from an approver in each of these files:
+
+- ~~[a/OWNERS](https://github.com/org/repo/blob/master/a/OWNERS)~~ [Alice]
+
 Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
 Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
 </details>

diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
@@ -289,6 +289,7 @@ func (a Approval) String() string {
 type Approvers struct {
 	owners          Owners
 	approvers       map[string]Approval // The keys of this map are normalized to lowercase.
+	changeRequested map[string]bool     // CHANGE_REQUESTED from any reviewer prevents submission
 	assignees       sets.Set[string]
 	AssociatedIssue int
 	RequireIssue    bool
@@ -316,9 +317,10 @@ func CaseInsensitiveIntersection(one, other sets.Set[string]) sets.Set[string] {
 // NewApprovers create a new "Approvers" with no approval.
 func NewApprovers(owners Owners) Approvers {
 	return Approvers{
-		owners:    owners,
-		approvers: map[string]Approval{},
-		assignees: sets.New[string](),
+		owners:          owners,
+		approvers:       map[string]Approval{},
+		changeRequested: map[string]bool{},
+		assignees:       sets.New[string](),
 
 		ManuallyApproved: func() bool {
 			return false
@@ -364,6 +366,11 @@ func (ap *Approvers) AddApprover(login, reference string, noIssue bool) {
 	}
 }
 
+// AddChangeRequested adds a new changeRequested
+func (ap *Approvers) AddChangeRequested(login string) {
+	ap.changeRequested[strings.ToLower(login)] = true
+}
+
 // AddAuthorSelfApprover adds the author self approval
 func (ap *Approvers) AddAuthorSelfApprover(login, reference string, noIssue bool) {
 	if ap.shouldNotOverrideApproval(login, noIssue) {
@@ -382,6 +389,11 @@ func (ap *Approvers) RemoveApprover(login string) {
 	delete(ap.approvers, strings.ToLower(login))
 }
 
+// RemoveChangeRequested removes a changeRequested from the map.
+func (ap *Approvers) RemoveChangeRequested(login string) {
+	delete(ap.changeRequested, strings.ToLower(login))
+}
+
 // AddAssignees adds assignees to the list
 func (ap *Approvers) AddAssignees(logins ...string) {
 	for _, login := range logins {
@@ -542,6 +554,12 @@ func (ap Approvers) GetCCs() []string {
 	return sets.List(suggested.Union(keepAssignees))
 }
 
+// AreChangesRequested returns a bool indicating whether changes are requested on the PR.
+// A PR cannot be submitted if there is an outstanding CHANGES_REQUESTED state.
+func (ap Approvers) AreChangesRequested() bool {
+	return len(ap.changeRequested) > 0
+}
+
 // AreFilesApproved returns a bool indicating whether or not OWNERS files associated with
 // the PR are approved.  A PR with no OWNERS files is not considered approved. If this
 // returns true, the PR may still not be fully approved depending on the associated issue
@@ -557,7 +575,7 @@ func (ap Approvers) AreFilesApproved() bool {
 //   - that there is an associated issue with the PR
 //   - an OWNER has indicated that the PR is trivial enough that an issue need not be associated with the PR
 func (ap Approvers) RequirementsMet() bool {
-	return ap.AreFilesApproved() && (!ap.RequireIssue || ap.AssociatedIssue != 0 || len(ap.NoIssueApprovers()) != 0)
+	return !ap.AreChangesRequested() && ap.AreFilesApproved() && (!ap.RequireIssue || ap.AssociatedIssue != 0 || len(ap.NoIssueApprovers()) != 0)
 }
 
 // IsApproved returns a bool indicating whether the PR is fully approved.