Allow anyone to approve #117
Conversation
Anyone can approve except the workflow initiator if they're not allowed.
fix length of issue approvers
|
What kind of testing did you do for this PR? And also thanks for contributing! |
|
bump, this would be a nice feature :) |
I've been using this internally with no issues, but honestly we always have it set to allow all reviewers since it's private repos. |
|
Hello, bumping as it'd be really helpful for me. Is there anything blocking us from merging it? |
| @@ -6,7 +6,7 @@ branding: | |||
| inputs: | |||
| approvers: | |||
| approverIdx := approversIndex(remainingApprovers, commentUser) | ||
| if approverIdx < 0 { | ||
|
|
||
| if approversIndex(disallowedUsers, commentUser) >= 0 { |
There was a problem hiding this comment.
I think nonApprovers is a little more concise and clear
| @@ -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) | |||
There was a problem hiding this comment.
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
| @@ -6,7 +6,7 @@ branding: | |||
| inputs: | |||
| approvers: | |||
| description: Required approvers | |||
| required: true | |||
| required: false | |||
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
Instead of overwriting a variable, it would be better to use if...else.
| } | ||
|
|
||
| approvers := []string{} | ||
| requiredApproversRaw := os.Getenv(envVarApprovers) | ||
| requiredApprovers := strings.Split(requiredApproversRaw, ",") | ||
| requiredApprovers := []string{} |
There was a problem hiding this comment.
Instead of overwriting a variable, it would be better to use if...else.
|
Also, please rebase on the main branch to account the new changes. |
Make the
approversfield optional, and allow anyone to approve if it's empty.I had to rework
approvalFromCommentsto make this work, since it worked by removing names from the required approvers list until the list was small enough.closes #108