-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding BypassPullRequestActorIDs to branch protection #1030
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can we get this shipped please? :-) |
@jcudit please could you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few initial questions. I like this feature addition a lot.
Can you add tests to cover this change?
@@ -37,6 +37,14 @@ func githubv4IDSlice(ss []string) []githubv4.ID { | |||
return vGh4 | |||
} | |||
|
|||
func githubv4IDSliceEmpty(ss []string) []githubv4.ID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is "Empty" in the name here? It looks like the function is a translation between a string slice and a githubv4.ID slice, and I'm not quite understanding how Empty fits in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember it correctly, I had to add this here so we can set the list of pull_request_bypassers
to empty list:
resource "github_branch_protection" "test" {
...
required_pull_request_reviews {
pull_request_bypassers = []
}
}
Without this we cannot remove all bypassers if any were set before.
I didn't want to make changes to the githubv4IDSlice
for the case some other logic would break so I added this new function. If you feel the function should be called differently, plese suggest a new name and I will amend it.
if a.Actor.Team != (Actor{}) { | ||
bypassActors = append(bypassActors, a.Actor.Team.ID.(string)) | ||
} | ||
if a.Actor.User != (Actor{}) { | ||
bypassActors = append(bypassActors, a.Actor.User.ID.(string)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind explaining this logic to me a little more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is the same like in the case of setPushActorIDs
bellow.
Any update on this? I am very interested in having this merged :) |
Also interested in seeing this get merged, pretty please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to merge!
Thanks a ton for submitting this functionality. |
Any idea when this will be merged? |
Hi, |
@chenshap I didn't have immediate plans to do so, but if you'd like to open a PR I'd be receptive to reviewing it and getting it in! |
This PR is fixing the issue #1005 by adding the functionality to define a list of users/teams that can bypass the PR restrictions. Unfortunately there doesn't seem to be any boolean flag like in the case of the
restrict_dismissals
+dismissal_restrictions
parameters.