-
Notifications
You must be signed in to change notification settings - Fork 764
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
Add required_approving_review_count #89 (required go-github update) #160
Add required_approving_review_count #89 (required go-github update) #160
Conversation
@radeksimko Hey! no rush or anything but I opened this PR. Don't let the XXL scare you from reviewing it, it is very small except it contains an update to the Github SDK. ACC tests passed locally and we implemented this change in our stack and it works great! Thanks for any input!! |
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
Description: "Number of approvals required to merge a pull request", | ||
Default: 1, | ||
// Old version of terraform, could not utilize validation.IntBetween | ||
ValidateFunc: func(i interface{}, k string) (s []string, es []error) { |
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.
You can use:
import "github.com/hashicorp/terraform/helper/validation"
...
ValidateFunc: validation.IntBetween(1, 6),
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.
Will be needed to add this on vendor.json
{
"checksumSHA1": "nEC56vB6M60BJtGPe+N9rziHqLg=",
"path": "github.com/hashicorp/terraform/helper/validation",
"revision": "41e50bd32a8825a84535e353c3674af8ce799161",
"version": "v0.11.7",
"versionExact": "v0.11.7"
},
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.
@@ -205,7 +220,7 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} | |||
} | |||
|
|||
if err := flattenAndSetRequiredPullRequestReviews(d, githubProtection); err != nil { | |||
return fmt.Errorf("Error setting required_pull_request_reviews: %v", err) | |||
return fmt.Errorf("Error setting required_pull_reuest_reviews: %v", err) |
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.
typo: required_pull_request_reviews
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.
Hmm looks like that was an old diff, I see it fixed here 9680382
Still new to govendor, they now match other terraform packages.
Any news @majormoses ? |
I am not a maintainer for this project and my golang foo is pretty weak I just reviewed it saying I didnt see anything that looked incredibly stupid. |
Any updates on getting this reviewed? |
@bflad I noticed you merged the last PR, this has been open for a while, any hope of getting this reviewed? I have been using it and it is quite nice. No worries if not, mostly just curious if anything is holding it up. |
Looking forward to seeing this approved. |
Looking forward to seeing this approved also, this is one of the several issues we need to solve before migrating to terraform GitHub management. |
Any updates on getting this reviewed? |
1 similar comment
Any updates on getting this reviewed? |
Looking forward to this! |
|
Fixes #89