Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ branding:
inputs:
approvers:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add your input here

description: Required approvers
required: true
required: false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest keeping this field as required and adding a keyword for your use case.
If we have a keyword instead of an empty field it could save potential security catastrophies caused by a mere oversight.

secret:
description: Secret
required: true
Expand All @@ -21,7 +21,7 @@ inputs:
required: false
exclude-workflow-initiator-as-approver:
description: Whether or not to filter out the user who initiated the workflow as an approver if they are in the approvers list
default: false
default: 'false'
additional-approved-words:
description: Comma separated list of words that can be used to approve beyond the defaults.
default: ''
Expand Down
35 changes: 25 additions & 10 deletions approval.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ type approvalEnvironment struct {
issueTitle string
issueBody string
issueApprovers []string
disallowedUsers []string
minimumApprovals int
}

func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner string, runID int, approvers []string, minimumApprovals int, issueTitle, issueBody string) (*approvalEnvironment, error) {
func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner string, runID int, approvers []string, minimumApprovals int, issueTitle, issueBody string, disallowedUsers []string) (*approvalEnvironment, error) {
repoOwnerAndName := strings.Split(repoFullName, "/")
if len(repoOwnerAndName) != 2 {
return nil, fmt.Errorf("repo owner and name in unexpected format: %s", repoFullName)
Expand All @@ -37,6 +38,7 @@ func newApprovalEnvironment(client *github.Client, repoFullName, repoOwner strin
repoOwner: repoOwner,
runID: runID,
issueApprovers: approvers,
disallowedUsers: disallowedUsers,
minimumApprovals: minimumApprovals,
issueTitle: issueTitle,
issueBody: issueBody,
Expand All @@ -54,14 +56,19 @@ func (a *approvalEnvironment) createApprovalIssue(ctx context.Context) error {
issueTitle = fmt.Sprintf("%s: %s", issueTitle, a.issueTitle)
}

issueApproversText := "Anyone can approve."
if len(a.issueApprovers) > 0 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of overwriting a variable, it would be better to use if...else.

issueApproversText = fmt.Sprintf("%s", a.issueApprovers)
}

issueBody := fmt.Sprintf(`Workflow is pending manual review.
URL: %s

Required approvers: %s

Respond %s to continue workflow or %s to cancel.`,
a.runURL(),
a.issueApprovers,
issueApproversText,
formatAcceptedWords(approvedWords),
formatAcceptedWords(deniedWords),
)
Expand Down Expand Up @@ -93,18 +100,27 @@ Respond %s to continue workflow or %s to cancel.`,
return nil
}

func approvalFromComments(comments []*github.IssueComment, approvers []string, minimumApprovals int) (approvalStatus, error) {
remainingApprovers := make([]string, len(approvers))
copy(remainingApprovers, approvers)
func approvalFromComments(comments []*github.IssueComment, approvers []string, minimumApprovals int, disallowedUsers []string) (approvalStatus, error) {

approvals := []string{}

if minimumApprovals == 0 {
if len(approvers) == 0 {
return "", fmt.Errorf("error: no required approvers or minimum approvals set")
}
minimumApprovals = len(approvers)
}

for _, comment := range comments {
commentUser := comment.User.GetLogin()
approverIdx := approversIndex(remainingApprovers, commentUser)
if approverIdx < 0 {

if approversIndex(disallowedUsers, commentUser) >= 0 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nonApprovers is a little more concise and clear

continue
}
if approversIndex(approvals, commentUser) >= 0 {
continue
}
if len(approvers) > 0 && approversIndex(approvers, commentUser) < 0 {
continue
}

Expand All @@ -114,11 +130,10 @@ func approvalFromComments(comments []*github.IssueComment, approvers []string, m
return approvalStatusPending, err
}
if isApprovalComment {
if len(remainingApprovers) == len(approvers)-minimumApprovals+1 {
approvals = append(approvals, commentUser)
if len(approvals) >= minimumApprovals {
return approvalStatusApproved, nil
}
remainingApprovers[approverIdx] = remainingApprovers[len(remainingApprovers)-1]
remainingApprovers = remainingApprovers[:len(remainingApprovers)-1]
continue
}

Expand Down
3 changes: 2 additions & 1 deletion approval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ func TestApprovalFromComments(t *testing.T) {
name string
comments []*github.IssueComment
approvers []string
disallowedUsers []string
minimumApprovals int
expectedStatus approvalStatus
}{
Expand Down Expand Up @@ -162,7 +163,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.disallowedUsers)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like to see a test for this specifically - as well as a test for if the same user is in the disallowed and allowed lists

if err != nil {
t.Fatalf("error getting approval from comments: %v", err)
}
Expand Down
31 changes: 23 additions & 8 deletions approvers.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,34 @@ import (
"github.com/google/go-github/v43/github"
)

func retrieveApprovers(client *github.Client, repoOwner string) ([]string, error) {
func retrieveApprovers(client *github.Client, repoOwner string) ([]string, []string, error) {
workflowInitiator := os.Getenv(envVarWorkflowInitiator)
shouldExcludeWorkflowInitiatorRaw := os.Getenv(envVarExcludeWorkflowInitiatorAsApprover)
shouldExcludeWorkflowInitiator, parseBoolErr := strconv.ParseBool(shouldExcludeWorkflowInitiatorRaw)
if parseBoolErr != nil {
return nil, fmt.Errorf("error parsing exclude-workflow-initiator-as-approver flag: %w", parseBoolErr)
return nil, nil, fmt.Errorf("error parsing exclude-workflow-initiator-as-approver flag: %w", parseBoolErr)
}

approvers := []string{}
requiredApproversRaw := os.Getenv(envVarApprovers)
requiredApprovers := strings.Split(requiredApproversRaw, ",")
requiredApprovers := []string{}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of overwriting a variable, it would be better to use if...else.

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
}

for i := range requiredApprovers {
requiredApprovers[i] = strings.TrimSpace(requiredApprovers[i])
Expand All @@ -39,21 +56,19 @@ func retrieveApprovers(client *github.Client, repoOwner string) ([]string, error

approvers = deduplicateUsers(approvers)

minimumApprovalsRaw := os.Getenv(envVarMinimumApprovals)
minimumApprovals := len(approvers)
var err error
if minimumApprovalsRaw != "" {
minimumApprovals, err = strconv.Atoi(minimumApprovalsRaw)
if err != nil {
return nil, fmt.Errorf("error parsing minimum number of approvals: %w", err)
return nil, nil, fmt.Errorf("error parsing minimum number of approvals: %w", err)
}
}

if minimumApprovals > len(approvers) {
return nil, fmt.Errorf("error: minimum required approvals (%d) is greater than the total number of approvers (%d)", 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 approvers, nil
return approvers, disallowedUsers, nil
}

func expandGroupFromUser(client *github.Client, org, userOrTeam string, workflowInitiator string, shouldExcludeWorkflowInitiator bool) []string {
Expand Down
10 changes: 3 additions & 7 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func newCommentLoopChannel(ctx context.Context, apprv *approvalEnvironment, clie
close(channel)
}

approved, err := approvalFromComments(comments, apprv.issueApprovers, apprv.minimumApprovals)
approved, err := approvalFromComments(comments, apprv.issueApprovers, apprv.minimumApprovals, apprv.disallowedUsers)
if err != nil {
fmt.Printf("error getting approval from comments: %v\n", err)
channel <- 1
Expand Down Expand Up @@ -133,10 +133,6 @@ func validateInput() error {
missingEnvVars = append(missingEnvVars, envVarToken)
}

if os.Getenv(envVarApprovers) == "" {
missingEnvVars = append(missingEnvVars, envVarApprovers)
}

if len(missingEnvVars) > 0 {
return fmt.Errorf("missing env vars: %v", missingEnvVars)
}
Expand Down Expand Up @@ -164,7 +160,7 @@ func main() {
os.Exit(1)
}

approvers, err := retrieveApprovers(client, repoOwner)
approvers, disallowedUsers, err := retrieveApprovers(client, repoOwner)
if err != nil {
fmt.Printf("error retrieving approvers: %v\n", err)
os.Exit(1)
Expand All @@ -181,7 +177,7 @@ func main() {
os.Exit(1)
}
}
apprv, err := newApprovalEnvironment(client, repoFullName, repoOwner, runID, approvers, minimumApprovals, issueTitle, issueBody)
apprv, err := newApprovalEnvironment(client, repoFullName, repoOwner, runID, approvers, minimumApprovals, issueTitle, issueBody, disallowedUsers)
if err != nil {
fmt.Printf("error creating approval environment: %v\n", err)
os.Exit(1)
Expand Down