option: allow non author contributor #457
Conversation
There was a problem hiding this comment.
Thanks for implementing this. If I could go back in time (or if we make a v2 config), I would make allow_author and allow_contributor have the expected behavior, but since we're stuck with that mistake for now, the new allow_non_author_contributor option sounds good.
I don't think the current implementation is correct though. While your new tests seem to cover all the cases, I believe two of them have wrong assertions. If you can fix those up and then update the approval code so they pass, this should be good.
policy/approval/approve_test.go
Outdated
| }, | ||
| }, | ||
| } | ||
| assertApproved(t, prctx, r, "Approved by comment-approver, review-approver") |
There was a problem hiding this comment.
I believe this should should approvals by contributor-author and contributor-committer. Both are non-author contributors on the PR who left approvals.
There was a problem hiding this comment.
oh yea forgot about the +1 default
policy/approval/approve_test.go
Outdated
| }, | ||
| }, | ||
| } | ||
| assertApproved(t, prctx, r, "Approved by comment-approver, mhaypenny, review-approver") |
There was a problem hiding this comment.
Same here, while this allows the author, it's missing the approvals from the contributors.
policy/approval/approve.go
Outdated
| @@ -189,6 +190,11 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string | |||
| // "author" is the user who opened the PR | |||
| // if contributors are allowed, the author counts as a contributor | |||
| author := prctx.Author() | |||
|
|
|||
| if !r.Options.AllowAuthor && !r.Options.AllowNonAuthorContributor && !r.Options.AllowContributor { | |||
There was a problem hiding this comment.
I don't think this condition is the correct one. If I'm reading this right, this combines with line 198 to reduce to something like:
if (a && b && c) || (a && b) {
banned[author] = true
}That if condition reduces further to a && b, which is what the old code checks.
Instead of changing when the author is banned, I think we need to change when contributors are banned based on the setting of the flag (so some modification to the block starting on line 203.)
There was a problem hiding this comment.
ahh gotcha, yea this was hard to grok, will get this fixed up.
|
Hello, Any plans for the next release that might include this feature? |
|
Hey @craigbryson, we're definitely overdue for a new release. There are two more in progress PRs that I'd like to include which I expect to merge this week. With testing, I think we should be able to make a release early next week. In the meantime, if you are using the Docker image, you can pull the |
|
Hi @bluekeyes, that's great, we're in no rush as you said we've just copied todays snapshot to our CR and it looks to be working fine 😄 |
Closes: #133
Similar to @ajlake, we ran into this today when trying to define a particular rule that could be approved by someone who had worked (by commits not suggested commits) with an individual on a PR but was not the PR author. After trying the combination below, I was pretty surprised to find in the documentation that a user could approve their own PR even if
allow_author: false. This PR attempts to correct that.Maybe this is actually desirable behavior for people using a comment driven workflow, but it differs greatly from a review driven workflow where an author isn't allowed to review their own PR anyway. So I think for the folks that prefer to use github review approval, we need a way to say allow contributors that doesn't include the author. Although the existing option would work, what if I decided later I want to use both github reviews and comment patterns then I'd get surprising behavior.
Adding this new option, allows us to not break the existing behavior for anyone who was currently using it this way while adding additional functionality for comment driven workflows. I tried to add every test case I could think of to make sure I got the logic right. Let me know if I missed something or if there's a better name for the option.