diff --git a/README.md b/README.md index cf58acd..74383b1 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ steps: additional-approved-words: '' additional-denied-words: '' polling-interval-seconds: 10 + allow-comment-reasons: false ``` * `approvers` is a comma-delimited list of all required approvers. An approver can either be a user or an org team. (*Note: Required approvers must have the ability to be set as approvers in the repository. If you add an approver that doesn't have this permission then you would receive an HTTP/402 Validation Failed error when running this action*) @@ -69,6 +70,8 @@ steps: * `additional-denied-words` is a comma separated list of strings to expand the dictionary of words that indicate denial. This is optional and defaults to an empty string. * `polling-interval-seconds` is an integer that sets the number of seconds to wait between polling the GitHub API for approval status. This is optional and defaults to `10` seconds. Increase this value if you want to reduce API calls, or decrease it for faster response times. +* `allow-comment-reasons` allows an approver to put the approval or denial keyword on the first line and explanatory text on later lines. It defaults to `false`, preserving the existing requirement that the whole comment contain only the decision keyword and optional punctuation. When enabled, only the first line determines the decision. + > [!Note] > 1. If You are using issue-body-file-path then please make sure the file is reachable; for example, if the file is in your repo, then please checkout to your repo in the same job as the approval issue. > 2. When using issue-body, the content string is passed as an arguent which is limited by github at 10kb. For content >= 10kb, use files for passing the issue body. diff --git a/action.yaml b/action.yaml index 22f4e3c..4574331 100644 --- a/action.yaml +++ b/action.yaml @@ -57,6 +57,12 @@ inputs: comment will be treated as a denial. Disabled by default. required: false default: "false" + allow-comment-reasons: + description: > + If true, treat an approval or denial keyword on the first line as the + decision and allow explanatory text on later lines. + required: false + default: "false" outputs: issue-number: description: The number of the issue created diff --git a/approval.go b/approval.go index 678a3f7..6cbedfa 100644 --- a/approval.go +++ b/approval.go @@ -27,9 +27,10 @@ type approvalEnvironment struct { targetRepoName string failOnDenial bool closeIssueMeansDenial bool + allowCommentReasons bool } -func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner string, runID int, approvers []string, minimumApprovals int, issueTitle, issueBody string, targetRepoOwner string, targetRepoName string, failOnDenial bool, closeIssueMeansDenial bool, issueLabels []string) (*approvalEnvironment, error) { +func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner string, runID int, approvers []string, minimumApprovals int, issueTitle, issueBody string, targetRepoOwner string, targetRepoName string, failOnDenial bool, closeIssueMeansDenial bool, allowCommentReasons bool, issueLabels []string) (*approvalEnvironment, error) { repoOwnerAndName := strings.Split(repoFullName, "/") if len(repoOwnerAndName) != 2 { return nil, fmt.Errorf("repo owner and name in unexpected format: %s", repoFullName) @@ -50,6 +51,7 @@ func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner strin targetRepoName: targetRepoName, failOnDenial: failOnDenial, closeIssueMeansDenial: closeIssueMeansDenial, + allowCommentReasons: allowCommentReasons, issueLabels: issueLabels, }, nil } @@ -184,7 +186,7 @@ func (a *approvalEnvironment) SetActionOutputs(outputs map[string]string) (bool, return true, nil } -func approvalFromComments(comments []*github.IssueComment, approvers []string, minimumApprovals int) (approvalStatus, error) { +func approvalFromComments(comments []*github.IssueComment, approvers []string, minimumApprovals int, allowCommentReasons bool) (approvalStatus, error) { remainingApprovers := make([]string, len(approvers)) copy(remainingApprovers, approvers) @@ -199,7 +201,7 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m continue } - commentBody := comment.GetBody() + commentBody := decisionText(comment.GetBody(), allowCommentReasons) isApprovalComment, err := isApproved(commentBody) if err != nil { return approvalStatusPending, err @@ -225,6 +227,20 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m return approvalStatusPending, nil } +// decisionText keeps the existing exact-comment behavior unless the optional +// comment-reason mode is enabled. In that mode only the first line determines +// the decision, so later lines can explain it without introducing ambiguous +// approval or denial keywords. +func decisionText(commentBody string, allowCommentReasons bool) string { + if !allowCommentReasons { + return commentBody + } + + normalized := strings.ReplaceAll(commentBody, "\r\n", "\n") + firstLine, _, _ := strings.Cut(normalized, "\n") + return firstLine +} + func approversIndex(approvers []string, name string) int { for idx, approver := range approvers { if strings.EqualFold(approver, name) { diff --git a/approval_test.go b/approval_test.go index 421cd22..5d8bd4f 100644 --- a/approval_test.go +++ b/approval_test.go @@ -1,14 +1,155 @@ package main import ( + "context" + "encoding/json" "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" "os" "strings" + "sync" "testing" + "time" "github.com/google/go-github/v43/github" ) +func TestApprovalFromCommentsAllowCommentReasons(t *testing.T) { + comment := func(user, body string) *github.IssueComment { + return &github.IssueComment{User: &github.User{Login: github.String(user)}, Body: github.String(body)} + } + testCases := []struct { + name string + comments []*github.IssueComment + minimum int + enabled approvalStatus + disabled approvalStatus + }{ + {"approval_with_reason", []*github.IssueComment{comment("login1", "Approved.\nDenied is explanatory text only.")}, 1, approvalStatusApproved, approvalStatusPending}, + {"denial_with_crlf_reason", []*github.IssueComment{comment("login1", "Denied!\r\nApproved is explanatory text only.")}, 1, approvalStatusDenied, approvalStatusPending}, + {"exact_approval", []*github.IssueComment{comment("LOGIN1", "APPROVED!\n")}, 1, approvalStatusApproved, approvalStatusApproved}, + {"exact_denial", []*github.IssueComment{comment("login1", "Denied.\n")}, 1, approvalStatusDenied, approvalStatusDenied}, + {"decision_on_later_line", []*github.IssueComment{comment("login1", "Context first.\nApproved.")}, 1, approvalStatusPending, approvalStatusPending}, + {"empty_first_line", []*github.IssueComment{comment("login1", "\nApproved.")}, 1, approvalStatusPending, approvalStatusPending}, + {"same_line_explanation", []*github.IssueComment{comment("login1", "Approved. Checks passed.")}, 1, approvalStatusPending, approvalStatusPending}, + {"unauthorized_approval", []*github.IssueComment{comment("outsider", "Approved.\nChecks passed.")}, 1, approvalStatusPending, approvalStatusPending}, + {"unauthorized_denial", []*github.IssueComment{comment("outsider", "Denied.\nChecks failed.")}, 1, approvalStatusPending, approvalStatusPending}, + {"distinct_approvers", []*github.IssueComment{comment("login1", "Approved.\nFirst review."), comment("login2", "Approved.\nSecond review.")}, 2, approvalStatusApproved, approvalStatusPending}, + {"duplicate_approver", []*github.IssueComment{comment("login1", "Approved.\nFirst review."), comment("login1", "Approved.\nRepeated review.")}, 2, approvalStatusPending, approvalStatusPending}, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + for _, allowReasons := range []bool{false, true} { + t.Run(fmt.Sprintf("allow_reasons=%t", allowReasons), func(t *testing.T) { + expected := testCase.disabled + if allowReasons { + expected = testCase.enabled + } + actual, err := approvalFromComments(testCase.comments, []string{"login1", "login2"}, testCase.minimum, allowReasons) + if err != nil || actual != expected { + t.Fatalf("got status %s, error %v; want %s", actual, err, expected) + } + }) + } + }) + } +} + +func TestCommentLoopAllowCommentReasons(t *testing.T) { + for _, allowReasons := range []bool{false, true} { + for _, decision := range []string{"Approved", "Denied"} { + t.Run(fmt.Sprintf("allow_reasons=%t/%s", allowReasons, decision), func(t *testing.T) { + var mu sync.Mutex + polls, closingComments, closes := 0, 0, 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + w.Header().Set("Content-Type", "application/json") + var response any + switch { + case r.Method == http.MethodGet && r.URL.Path == "/repos/owner/repo/issues/1/comments": + polls++ + comments := []*github.IssueComment{{ + User: &github.User{Login: github.String("login1")}, + Body: github.String(decision + ".\nThe deployment checks explain this decision."), + }} + if polls > 1 { + comments = append(comments, &github.IssueComment{ + User: &github.User{Login: github.String("login1")}, Body: github.String(decision), + }) + } + response = comments + case r.Method == http.MethodPost && r.URL.Path == "/repos/owner/repo/issues/1/comments": + var comment github.IssueComment + wantBody := "The required number of approvals (1) has been met; continuing workflow and closing this issue." + if decision == "Denied" { + wantBody = "Request denied. Closing issue and failing workflow." + } + if err := json.NewDecoder(r.Body).Decode(&comment); err != nil || comment.GetBody() != wantBody { + t.Errorf("got closing comment %q, error %v; want %q", comment.GetBody(), err, wantBody) + } + closingComments++ + response = &github.IssueComment{} + case r.Method == http.MethodPatch && r.URL.Path == "/repos/owner/repo/issues/1": + var issue github.IssueRequest + if err := json.NewDecoder(r.Body).Decode(&issue); err != nil || issue.GetState() != "closed" { + t.Errorf("got close request %v, error %v", issue, err) + } + closes++ + response = &github.Issue{} + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected request", http.StatusNotFound) + return + } + if err := json.NewEncoder(w).Encode(response); err != nil { + t.Errorf("encoding response: %v", err) + } + })) + defer server.Close() + client := github.NewClient(server.Client()) + baseURL, err := url.Parse(server.URL + "/") + if err != nil { + t.Fatal(err) + } + client.BaseURL = baseURL + apprv := &approvalEnvironment{ + targetRepoOwner: "owner", targetRepoName: "repo", approvalIssueNumber: 1, + issueApprovers: []string{"login1"}, minimumApprovals: 1, + failOnDenial: true, allowCommentReasons: allowReasons, + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + result := newCommentLoopChannel(ctx, apprv, client, time.Millisecond) + select { + case exit := <-result: + wantExit := 0 + if decision == "Denied" { + wantExit = 1 + } + if exit != wantExit { + t.Fatalf("got exit %d, want %d", exit, wantExit) + } + case <-ctx.Done(): + t.Fatal("comment loop did not complete") + } + mu.Lock() + defer mu.Unlock() + wantPolls := 2 + if allowReasons { + wantPolls = 1 + } + if polls != wantPolls || closingComments != 1 || closes != 1 { + t.Fatalf("got polls/comments/closes %d/%d/%d; want %d/1/1", polls, closingComments, closes, wantPolls) + } + }) + } + } +} + func TestApprovalFromComments(t *testing.T) { login1 := "login1" login2 := "login2" @@ -25,6 +166,7 @@ func TestApprovalFromComments(t *testing.T) { approvers []string minimumApprovals int expectedStatus approvalStatus + allowReasons bool }{ { name: "single_approver_single_comment_approved", @@ -59,6 +201,53 @@ func TestApprovalFromComments(t *testing.T) { approvers: []string{login1}, expectedStatus: approvalStatusPending, }, + { + name: "single_approver_approval_with_reason_enabled", + comments: []*github.IssueComment{ + { + User: &github.User{Login: &login1}, + Body: github.String("Approved.\nA later denied keyword is explanatory text only."), + }, + }, + approvers: []string{login1}, + expectedStatus: approvalStatusApproved, + allowReasons: true, + }, + { + name: "single_approver_denial_with_reason_enabled", + comments: []*github.IssueComment{ + { + User: &github.User{Login: &login1}, + Body: github.String("Denied!\r\nA later approved keyword is explanatory text only."), + }, + }, + approvers: []string{login1}, + expectedStatus: approvalStatusDenied, + allowReasons: true, + }, + { + name: "single_approver_reason_stays_pending_by_default", + comments: []*github.IssueComment{ + { + User: &github.User{Login: &login1}, + Body: github.String("Approved.\nThe deployment checks passed."), + }, + }, + approvers: []string{login1}, + expectedStatus: approvalStatusPending, + }, + { + name: "decision_keyword_after_first_line_stays_pending", + comments: []*github.IssueComment{ + { + User: &github.User{Login: &login1}, + Body: github.String("Context first.\nApproved."), + }, + }, + approvers: []string{login1}, + expectedStatus: approvalStatusPending, + allowReasons: true, + }, { name: "single_approver_multi_comment_approved", comments: []*github.IssueComment{ @@ -178,7 +367,7 @@ func TestApprovalFromComments(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - actual, err := approvalFromComments(testCase.comments, testCase.approvers, testCase.minimumApprovals) + actual, err := approvalFromComments(testCase.comments, testCase.approvers, testCase.minimumApprovals, testCase.allowReasons) if err != nil { t.Fatalf("error getting approval from comments: %v", err) } diff --git a/constants.go b/constants.go index cbd6f0d..9b3753d 100644 --- a/constants.go +++ b/constants.go @@ -28,6 +28,7 @@ const ( envVarTargetRepo string = "INPUT_TARGET-REPOSITORY" envVarPollingIntervalSeconds string = "INPUT_POLLING-INTERVAL-SECONDS" envVarCloseIssueMeansDenial string = "INPUT_CLOSE-ISSUE-MEANS-DENIAL" + envVarAllowCommentReasons string = "INPUT_ALLOW-COMMENT-REASONS" ) var ( diff --git a/main.go b/main.go index 7b0f3a9..96c385e 100644 --- a/main.go +++ b/main.go @@ -61,7 +61,7 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie return } - approved, err := approvalFromComments(comments, apprv.issueApprovers, apprv.minimumApprovals) + approved, err := approvalFromComments(comments, apprv.issueApprovers, apprv.minimumApprovals, apprv.allowCommentReasons) if err != nil { fmt.Printf("error getting approval from comments: %v\n", err) channel <- 1 @@ -303,6 +303,16 @@ func main() { } } + allowCommentReasons := false + allowCommentReasonsRaw := os.Getenv(envVarAllowCommentReasons) + if allowCommentReasonsRaw != "" { + allowCommentReasons, err = strconv.ParseBool(allowCommentReasonsRaw) + if err != nil { + fmt.Printf("error parsing allow-comment-reasons: %v\n", err) + os.Exit(1) + } + } + pollingInterval := defaultPollingInterval pollingIntervalSecondsRaw := os.Getenv(envVarPollingIntervalSeconds) if pollingIntervalSecondsRaw != "" { @@ -349,7 +359,7 @@ func main() { } fmt.Printf("Parsed %d labels", len(issueLabels)) - apprv, err := newApprovalEnvironment(client, repoFullName, repoOwner, runID, approvers, minimumApprovals, issueTitle, issueBody, targetRepoOwner, targetRepoName, failOnDenial, closeIssueMeansDenial, issueLabels) + apprv, err := newApprovalEnvironment(client, repoFullName, repoOwner, runID, approvers, minimumApprovals, issueTitle, issueBody, targetRepoOwner, targetRepoName, failOnDenial, closeIssueMeansDenial, allowCommentReasons, issueLabels) if err != nil { fmt.Printf("error creating approval environment: %v\n", err) os.Exit(1)