From f08c475a1e71b0f522ea6fbf25966a9e4f739fad Mon Sep 17 00:00:00 2001 From: Philip Nicolcev Date: Mon, 2 Mar 2026 11:45:12 -0800 Subject: [PATCH] fix bugs, improve code quality, upgrade go-github to v74 - fix missing return after close(channel) in comment loop goroutine (would panic) - fix missing slash in githubBaseURL producing malformed run URLs - deduplicate isApproved/isDenied into single matchesWordList function - use strings.Join for readable approver formatting in issue bodies - use slices.Contains from stdlib instead of hand-rolled approversIndex - consolidate minimumApprovals parsing into retrieveApprovers (was parsed twice) - surface team-fetch errors in expandGroupFromUser instead of swallowing them - upgrade go-github from v61 to v74 Made-with: Cursor --- approval.go | 55 ++++++++++++------------------------------------ approval_test.go | 8 +++---- approvers.go | 49 +++++++++++++++++++++--------------------- constants.go | 2 +- go.mod | 2 +- go.sum | 8 +++---- main.go | 21 +++++++++--------- 7 files changed, 58 insertions(+), 87 deletions(-) diff --git a/approval.go b/approval.go index 8c675c4..1e41e89 100644 --- a/approval.go +++ b/approval.go @@ -4,9 +4,10 @@ import ( "context" "fmt" "regexp" + "slices" "strings" - "github.com/google/go-github/v61/github" + "github.com/google/go-github/v74/github" ) type approvalEnvironment struct { @@ -55,7 +56,7 @@ func (a *approvalEnvironment) createApprovalIssue(ctx context.Context) error { issueApproversText := "Anyone can approve." assignees := []string{a.workflowInitiator} if len(a.issueApprovers) > 0 { - issueApproversText = fmt.Sprintf("%s", a.issueApprovers) + issueApproversText = strings.Join(a.issueApprovers, ", ") assignees = a.issueApprovers } @@ -112,18 +113,18 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m for _, comment := range comments { commentUser := comment.User.GetLogin() - if approversIndex(disallowedUsers, commentUser) >= 0 { + if slices.Contains(disallowedUsers, commentUser) { continue } - if approversIndex(approvals, commentUser) >= 0 { + if slices.Contains(approvals, commentUser) { continue } - if len(approvers) > 0 && approversIndex(approvers, commentUser) < 0 { + if len(approvers) > 0 && !slices.Contains(approvers, commentUser) { continue } commentBody := comment.GetBody() - isApprovalComment, err := isApproved(commentBody) + isApprovalComment, err := matchesWordList(commentBody, approvedWords) if err != nil { return approvalStatusPending, err } @@ -135,7 +136,7 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m continue } - isDenialComment, err := isDenied(commentBody) + isDenialComment, err := matchesWordList(commentBody, deniedWords) if err != nil { return approvalStatusPending, err } @@ -147,46 +148,16 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m return approvalStatusPending, nil } -func approversIndex(approvers []string, name string) int { - for idx, approver := range approvers { - if approver == name { - return idx - } - } - return -1 -} - -func isApproved(commentBody string) (bool, error) { - for _, approvedWord := range approvedWords { - re, err := regexp.Compile(fmt.Sprintf("(?i)^%s[.!]*\n*\\s*$", regexp.QuoteMeta(approvedWord))) - if err != nil { - fmt.Printf("Error parsing. %v", err) - return false, err - } - - matched := re.MatchString(commentBody) - - if matched { - return true, nil - } - } - - return false, nil -} - -func isDenied(commentBody string) (bool, error) { - for _, deniedWord := range deniedWords { - re, err := regexp.Compile(fmt.Sprintf("(?i)^%s[.!]*\n*\\s*$", regexp.QuoteMeta(deniedWord))) +func matchesWordList(commentBody string, words []string) (bool, error) { + for _, word := range words { + re, err := regexp.Compile(fmt.Sprintf("(?i)^%s[.!]*\n*\\s*$", regexp.QuoteMeta(word))) if err != nil { - fmt.Printf("Error parsing. %v", err) - return false, err + return false, fmt.Errorf("error compiling pattern for %q: %w", word, err) } - matched := re.MatchString(commentBody) - if matched { + if re.MatchString(commentBody) { return true, nil } } - return false, nil } diff --git a/approval_test.go b/approval_test.go index 16b2e28..1c99e5c 100644 --- a/approval_test.go +++ b/approval_test.go @@ -3,7 +3,7 @@ package main import ( "testing" - "github.com/google/go-github/v61/github" + "github.com/google/go-github/v74/github" ) func TestApprovalFromComments(t *testing.T) { @@ -337,7 +337,7 @@ func TestApprovedCommentBody(t *testing.T) { } // test - actual, err := isApproved(testCase.commentBody) + actual, err := matchesWordList(testCase.commentBody, approvedWords) if err != nil { t.Fatalf("error getting approval: %v", err) } @@ -473,9 +473,9 @@ func TestDeniedCommentBody(t *testing.T) { } // test - actual, err := isDenied(testCase.commentBody) + actual, err := matchesWordList(testCase.commentBody, deniedWords) if err != nil { - t.Fatalf("error getting approval: %v", err) + t.Fatalf("error getting denial: %v", err) } if actual != testCase.isSuccess { t.Fatalf("expected %v but got %v", testCase.isSuccess, actual) diff --git a/approvers.go b/approvers.go index a216929..8d3558f 100644 --- a/approvers.go +++ b/approvers.go @@ -7,36 +7,31 @@ import ( "strconv" "strings" - "github.com/google/go-github/v61/github" + "github.com/google/go-github/v74/github" ) -func retrieveApprovers(client *github.Client, repoOwner string) ([]string, []string, error) { +func retrieveApprovers(client *github.Client, repoOwner string) ([]string, []string, int, error) { workflowInitiator := os.Getenv(envVarWorkflowInitiator) shouldExcludeWorkflowInitiatorRaw := os.Getenv(envVarExcludeWorkflowInitiatorAsApprover) shouldExcludeWorkflowInitiator, parseBoolErr := strconv.ParseBool(shouldExcludeWorkflowInitiatorRaw) if parseBoolErr != nil { - return nil, nil, fmt.Errorf("error parsing exclude-workflow-initiator-as-approver flag: %w", parseBoolErr) + return nil, nil, 0, fmt.Errorf("error parsing exclude-workflow-initiator-as-approver flag: %w", parseBoolErr) } - approvers := []string{} + var approvers []string requiredApproversRaw := os.Getenv(envVarApprovers) - requiredApprovers := []string{} + var requiredApprovers []string if requiredApproversRaw != "" { requiredApprovers = strings.Split(requiredApproversRaw, ",") } - minimumApprovalsRaw := os.Getenv(envVarMinimumApprovals) - minimumApprovals := len(approvers) - var disallowedUsers []string if shouldExcludeWorkflowInitiator { disallowedUsers = []string{workflowInitiator} - } else { - disallowedUsers = []string{} } if len(requiredApprovers) == 0 { - return []string{}, disallowedUsers, nil + return nil, disallowedUsers, 0, nil } for i := range requiredApprovers { @@ -44,7 +39,10 @@ func retrieveApprovers(client *github.Client, repoOwner string) ([]string, []str } for _, approverUser := range requiredApprovers { - expandedUsers := expandGroupFromUser(client, repoOwner, approverUser, workflowInitiator, shouldExcludeWorkflowInitiator) + expandedUsers, err := expandGroupFromUser(client, repoOwner, approverUser, workflowInitiator, shouldExcludeWorkflowInitiator) + if err != nil { + return nil, nil, 0, fmt.Errorf("error resolving approver %q: %w", approverUser, err) + } if expandedUsers != nil { approvers = append(approvers, expandedUsers...) } else if strings.EqualFold(workflowInitiator, approverUser) && shouldExcludeWorkflowInitiator { @@ -56,33 +54,36 @@ func retrieveApprovers(client *github.Client, repoOwner string) ([]string, []str approvers = deduplicateUsers(approvers) - var err error - if minimumApprovalsRaw != "" { - minimumApprovals, err = strconv.Atoi(minimumApprovalsRaw) + minimumApprovals := len(approvers) + if raw := os.Getenv(envVarMinimumApprovals); raw != "" { + var err error + minimumApprovals, err = strconv.Atoi(raw) if err != nil { - return nil, nil, fmt.Errorf("error parsing minimum number of approvals: %w", err) + return nil, nil, 0, fmt.Errorf("error parsing minimum number of approvals: %w", err) } } if minimumApprovals > len(approvers) { - return nil, nil, fmt.Errorf("error: minimum required approvals (%d) is greater than the total number of approvers (%d)", minimumApprovals, len(approvers)) + return nil, nil, 0, fmt.Errorf("error: minimum required approvals (%d) is greater than the total number of approvers (%d)", minimumApprovals, len(approvers)) } - return approvers, disallowedUsers, nil + return approvers, disallowedUsers, minimumApprovals, nil } -func expandGroupFromUser(client *github.Client, org, userOrTeam string, workflowInitiator string, shouldExcludeWorkflowInitiator bool) []string { +func expandGroupFromUser(client *github.Client, org, userOrTeam string, workflowInitiator string, shouldExcludeWorkflowInitiator bool) ([]string, error) { fmt.Printf("Attempting to expand user %s/%s as a group (may not succeed)\n", org, userOrTeam) // GitHub replaces periods in the team name with hyphens. If a period is // passed to the request it would result in a 404. So we need to replace - // and occurrences with a hyphen. + // any occurrences with a hyphen. formattedUserOrTeam := strings.ReplaceAll(userOrTeam, ".", "-") - users, _, err := client.Teams.ListTeamMembersBySlug(context.Background(), org, formattedUserOrTeam, &github.TeamListTeamMembersOptions{}) + users, resp, err := client.Teams.ListTeamMembersBySlug(context.Background(), org, formattedUserOrTeam, &github.TeamListTeamMembersOptions{}) if err != nil { - fmt.Printf("%v\n", err) - return nil + if resp != nil && resp.StatusCode == 404 { + return nil, nil + } + return nil, fmt.Errorf("error expanding team %s/%s: %w", org, userOrTeam, err) } userNames := make([]string, 0, len(users)) @@ -95,7 +96,7 @@ func expandGroupFromUser(client *github.Client, org, userOrTeam string, workflow } } - return userNames + return userNames, nil } func deduplicateUsers(users []string) []string { diff --git a/constants.go b/constants.go index f8c1b17..23e20f9 100644 --- a/constants.go +++ b/constants.go @@ -22,7 +22,7 @@ const ( envVarAdditionalApprovedWords string = "INPUT_ADDITIONAL-APPROVED-WORDS" envVarAdditionalDeniedWords string = "INPUT_ADDITIONAL-DENIED-WORDS" - githubBaseURL string = "https://github.com" + githubBaseURL string = "https://github.com/" ) var ( diff --git a/go.mod b/go.mod index 2d88394..175442e 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.24.0 toolchain go1.24.7 require ( - github.com/google/go-github/v61 v61.0.0 + github.com/google/go-github/v74 v74.0.0 golang.org/x/oauth2 v0.31.0 ) diff --git a/go.sum b/go.sum index 9952d33..55caa06 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/google/go-github/v61 v61.0.0 h1:VwQCBwhyE9JclCI+22/7mLB1PuU9eowCXKY5pNlu1go= -github.com/google/go-github/v61 v61.0.0/go.mod h1:0WR+KmsWX75G2EbpyGsGmradjo3IiciuI4BmdVCobQY= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/go-github/v74 v74.0.0 h1:yZcddTUn8DPbj11GxnMrNiAnXH14gNs559AsUpNpPgM= +github.com/google/go-github/v74 v74.0.0/go.mod h1:ubn/YdyftV80VPSI26nSJvaEsTOnsjrxG3o9kJhcyak= github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= golang.org/x/oauth2 v0.31.0 h1:8Fq0yVZLh4j4YA47vHKFTa9Ew5XIrCP8LC6UeNZnLxo= diff --git a/main.go b/main.go index 3d6d69e..5a5807e 100644 --- a/main.go +++ b/main.go @@ -8,7 +8,7 @@ import ( "strconv" "time" - "github.com/google/go-github/v61/github" + "github.com/google/go-github/v74/github" "golang.org/x/oauth2" ) @@ -39,6 +39,7 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie fmt.Printf("error getting comments: %v\n", err) channel <- 1 close(channel) + return } approved, err := approvalFromComments(comments, apprv.issueApprovers, apprv.minimumApprovals, apprv.disallowedUsers) @@ -46,6 +47,7 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie fmt.Printf("error getting approval from comments: %v\n", err) channel <- 1 close(channel) + return } fmt.Printf("Workflow status: %s\n", approved) switch approved { @@ -59,16 +61,19 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie fmt.Printf("error commenting on issue: %v\n", err) channel <- 1 close(channel) + return } _, _, err = client.Issues.Edit(ctx, apprv.repoOwner, apprv.repo, apprv.approvalIssueNumber, &github.IssueRequest{State: &newState}) if err != nil { fmt.Printf("error closing issue: %v\n", err) channel <- 1 close(channel) + return } channel <- 0 fmt.Println("Workflow manual approval completed") close(channel) + return case approvalStatusDenied: newState := "closed" closeComment := "Request denied. Closing issue and failing workflow." @@ -79,15 +84,18 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie fmt.Printf("error commenting on issue: %v\n", err) channel <- 1 close(channel) + return } _, _, err = client.Issues.Edit(ctx, apprv.repoOwner, apprv.repo, apprv.approvalIssueNumber, &github.IssueRequest{State: &newState}) if err != nil { fmt.Printf("error closing issue: %v\n", err) channel <- 1 close(channel) + return } channel <- 1 close(channel) + return } time.Sleep(pollingInterval) @@ -160,7 +168,7 @@ func main() { os.Exit(1) } - approvers, disallowedUsers, err := retrieveApprovers(client, repoOwner) + approvers, disallowedUsers, minimumApprovals, err := retrieveApprovers(client, repoOwner) if err != nil { fmt.Printf("error retrieving approvers: %v\n", err) os.Exit(1) @@ -168,15 +176,6 @@ func main() { issueTitle := os.Getenv(envVarIssueTitle) issueBody := os.Getenv(envVarIssueBody) - minimumApprovalsRaw := os.Getenv(envVarMinimumApprovals) - minimumApprovals := 0 - if minimumApprovalsRaw != "" { - minimumApprovals, err = strconv.Atoi(minimumApprovalsRaw) - if err != nil { - fmt.Printf("error parsing minimum approvals: %v\n", err) - os.Exit(1) - } - } workflowInitiator := os.Getenv(envVarWorkflowInitiator) apprv, err := newApprovalEnvironment(client, repoFullName, repoOwner, runID, approvers, minimumApprovals, issueTitle, issueBody, disallowedUsers, workflowInitiator) if err != nil {