diff --git a/pkg/plugins/approve/approve.go b/pkg/plugins/approve/approve.go
index dbcc267838..8513497c2c 100644
--- 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
index 1b181a6ce6..fe95cbc822 100644
--- 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
`,
},
{
- 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
+
+`,
+ },
+ {
+ 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: *Alice*
+
+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)
+
+
+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
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index c86c6794ab..225a3dca26 100644
--- 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.