Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement #880

victoryNap · 2018-03-28T01:01:48Z

Added RequiredApprovingReviewCount to PullRequestReviewsEnforcement

Related to the following issue - #879

This count represents the number of approvals required on a pull request before it can be merged.

This will complete the model for the required_pull_request_reviews object which is returned by the GitHub API.

https://developer.github.com/v3/repos/branches/#get-branch-protection

gmlewis · 2018-03-28T16:56:16Z

Thanks, @victoryNap. Some general feedback before a full review.

First, we have to be very careful with "omitempty" and also with the documentation here. In the GitHub Developer API documentation, there is one place where the field "must be" provided and another place where the field "can be" provided. The latter case needs to have "omitempty" and must be a pointer to an int so that the field can intentionally not be sent.

Also, the documentation needs to state here that the field must be a value between 1 and 6 for it to be valid.

Finally, all comments should be complete sentences with a period at the end so that the resulting Godocs read correctly. For example, see:
https://godoc.org/github.com/google/go-github/github

I don't have time for a full review at the moment, but saw these issues at a quick glance. Please address these first and make sure your documentation also specifies when the field is required and when it is optional... then please ping me and I'll do a full review.

Thanks again!

…tests

dmitshur

This is looking better. I've posted some comments and questions inline.

dmitshur · 2018-03-30T19:26:26Z

github/repos.go

@@ -579,6 +581,9 @@ type PullRequestReviewsEnforcementRequest struct {
 	DismissStaleReviews bool `json:"dismiss_stale_reviews"`
 	// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
 	RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
+	// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
+	// Can be omitted.
+	RequiredApprovingReviewCount *int `json:"required_approving_review_count,omitempty"`


This parameter appears to be mandatory, not optional, here.

https://developer.github.com/v3/repos/branches/#update-branch-protection lists required_approving_review_count in a table labeled as:

The required_pull_request_reviews object must have the following keys

Emphasis on must.

So, it should be:

RequiredApprovingReviewCount int `json:"required_approving_review_count"`

That was my initial thought as well, according to the docs its required. That said, the request has worked thus far without having it?

The documentation at https://developer.github.com/v3/repos/branches/#update-branch-protection has this note (which links to https://developer.github.com/changes/2018-03-16-protected-branches-required-approving-reviews/):

It doesn't look like we're setting that application/vnd.github.luke-cage-preview+json custom Accept header, are we?

dmitshur · 2018-03-30T19:27:59Z

github/repos_test.go

@@ -555,7 +555,8 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {
 					{Slug: String("t"), ID: Int64(4)},
 				},
 			},
-			RequireCodeOwnerReviews: true,
+			RequireCodeOwnerReviews:      true,
+			RequiredApprovingReviewCount: 0,


Why is the value 3 in the JSON above:

"required_approving_review_count":3

But RequiredApprovingReviewCount is 0 here?

dmitshur · 2018-03-30T19:29:32Z

github/repos_test.go

@@ -629,7 +630,8 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) {
 					{Slug: String("tt"), ID: Int64(4)},
 				},
 			},
-			RequireCodeOwnerReviews: true,
+			RequireCodeOwnerReviews:      true,
+			RequiredApprovingReviewCount: 0,


Let's give it a value in the valid 1-6 range, since it's documented that way:

Use a number between 1 and 6.

If the value hasn't been set, by default, it comes back as 0. I'll update the documents.

dmitshur · 2018-03-30T19:29:47Z

github/repos_test.go

@@ -776,7 +778,8 @@ func TestRepositoriesService_GetPullRequestReviewEnforcement(t *testing.T) {
 				{Slug: String("t"), ID: Int64(2)},
 			},
 		},
-		RequireCodeOwnerReviews: true,
+		RequireCodeOwnerReviews:      true,
+		RequiredApprovingReviewCount: 0,


dmitshur · 2018-03-30T19:30:58Z

github/repos_test.go

@@ -851,7 +855,8 @@ func TestRepositoriesService_DisableDismissalRestrictions(t *testing.T) {
 			Users: []*User{},
 			Teams: []*Team{},
 		},
-		RequireCodeOwnerReviews: true,
+		RequireCodeOwnerReviews:      true,
+		RequiredApprovingReviewCount: 0,


dmitshur · 2018-03-30T19:34:47Z

github/repos_test.go

@@ -532,7 +532,7 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {

 		testMethod(t, r, "GET")
 		testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview)
-		fmt.Fprintf(w, `{"required_status_checks":{"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"dismissal_restrictions":{"users":[{"id":3,"login":"u"}],"teams":[{"id":4,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true},"enforce_admins":{"url":"/repos/o/r/branches/b/protection/enforce_admins","enabled":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
+		fmt.Fprintf(w, `{"required_status_checks":{"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"dismissal_restrictions":{"users":[{"id":3,"login":"u"}],"teams":[{"id":4,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true},"enforce_admins":{"url":"/repos/o/r/branches/b/protection/enforce_admins","enabled":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"required_approving_review_count":3}`)


I see why it's 0 below. It's because the "required_approving_review_count":3 entry is added in the wrong place here. It should be added inside the "required_pull_request_reviews":{...} object.

Perhaps it's a good idea to make the JSON multi-line and nicely formatted, so it's easier to read and modify.

…removed old repo protection header. Updated tests with new headers and valid values

victoryNap · 2018-05-10T16:24:10Z

@shurcooL @gmlewis When you get a minute, would you take a look at this?

gmlewis · 2018-05-11T00:21:59Z

github/github.go

@@ -51,9 +51,6 @@ const (
 	// https://developer.github.com/changes/2014-12-09-new-attributes-for-stars-api/
 	mediaTypeStarringPreview = "application/vnd.github.v3.star+json"

-	// https://developer.github.com/changes/2015-11-11-protected-branches-api/
-	mediaTypeProtectedBranchesPreview = "application/vnd.github.loki-preview+json"


That seems odd that there wasn't a GitHub Developer API announcement revoking this custom media type. (Unless maybe I missed it?)
The last one I see was: https://developer.github.com/changes/2016-06-27-protected-branches-api-update/
and the new docs only mention the new custom media type, so maybe this one silently went away?
I'm guessing that's how you interpret the current situation, @victoryNap, judging from the PR.

I also didn't see an update saying it was no longer a preview, but thats the assumption that I made, their documentation makes no reference to the previous preview header, only to the new one that I've added.

gmlewis

Just a couple tweaks needed.
Thank you, @victoryNap!
Have you been able to test these changes out on GitHub?

gmlewis · 2018-05-11T00:23:31Z

github/repos.go

+	// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
+	// Valid values are 1-6.
+	// Can be omitted.
+	RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`


If it can be omitted, then I think we need to make this type *int instead of int.

Issue with the comment, I forgot to update it. Its a required field.

Then in that case, should we get rid of the ,omitempty here and below?

gmlewis · 2018-05-11T00:23:46Z

github/repos.go

+	// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
+	// Valid values are 1 - 6.
+	// Can be omitted.
+	RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`


Same here: *int.

Same as above

victoryNap · 2018-05-30T17:55:06Z

@gmlewis I tested this on github, and it worked! What kind of logs would you like to see?

gmlewis · 2018-05-31T00:58:10Z

Thanks for testing it, @victoryNap! I don't need to see the logs.

gmlewis · 2018-05-31T01:05:40Z

github/repos_test.go

+		// TODO: remove custom Accept header when this API fully launches
+		testHeader(t, r, "Accept", mediaTypeRequiredApprovingReviewsPreview)
+		fmt.Fprintf(w, `{
+							"required_status_checks":{


I'm thinking we don't need the large amount of indentation here (and below).
What about un-indenting this entire region? Something like:

fmt.Fprintf(w, `{ "required_status_checks":{ "strict":true, "contexts":["continuous-integration"] }, "required_pull_request_reviews":{ ...

?

gmlewis

Thank you, @victoryNap! I'm thinking this is almost ready to merge. One question about the required params, and then we will want a second LGTM before merging.

gmlewis · 2018-06-01T14:16:23Z

github/repos.go

@@ -579,6 +582,9 @@ type PullRequestReviewsEnforcementRequest struct {
 	DismissStaleReviews bool `json:"dismiss_stale_reviews"`
 	// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
 	RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
+	// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
+	// Valid values are 1-6.
+	RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`


What are your thoughts, @victoryNap on this ,omitempty?
I'm thinking it should be removed since it is a required parameter.

gmlewis · 2018-06-01T14:16:33Z

github/repos.go

@@ -591,6 +597,9 @@ type PullRequestReviewsEnforcementUpdate struct {
 	DismissStaleReviews *bool `json:"dismiss_stale_reviews,omitempty"`
 	// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
 	RequireCodeOwnerReviews bool `json:"require_code_owner_reviews,omitempty"`
+	// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
+	// Valid values are 1 - 6.
+	RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`


...and this one too.

Also Updated

victoryNap · 2018-06-01T14:22:21Z

@gmlewis I don't know that having the block totally unindented looks makes sense, but I agree, I was overly aggressive with the tabs...

gmlewis · 2018-06-01T14:26:07Z

OK, @victoryNap, that's fine with me. In my own code, I like to left-justify the included JSON, but this looks much better now and I'm happy with that... thanks!

gmlewis

Thank you, @victoryNap!
This LGTM... awaiting second LGTM before merging.

gmlewis · 2018-06-01T14:29:40Z

It looks like we have a CI failure and some tests need updating:

--- FAIL: TestPullRequestReviewsEnforcementRequest_MarshalJSON_nilDismissalRestirctions (0.00s)
	repos_test.go:1034: PullRequestReviewsEnforcementRequest.MarshalJSON returned {"dismiss_stale_reviews":false,"require_code_owner_reviews":false,"required_approving_review_count":0}, want {"dismiss_stale_reviews":false,"require_code_owner_reviews":false}
	repos_test.go:1048: PullRequestReviewsEnforcementRequest.MarshalJSON returned {"dismissal_restrictions":{},"dismiss_stale_reviews":false,"require_code_owner_reviews":false,"required_approving_review_count":0}, want {"dismissal_restrictions":{},"dismiss_stale_reviews":false,"require_code_owner_reviews":false}
	repos_test.go:1065: PullRequestReviewsEnforcementRequest.MarshalJSON returned {"dismissal_restrictions":{"users":[],"teams":[]},"dismiss_stale_reviews":false,"require_code_owner_reviews":false,"required_approving_review_count":0}, want {"dismissal_restrictions":{"users":[],"teams":[]},"dismiss_stale_reviews":false,"require_code_owner_reviews":false}

Can you please look into this, @victoryNap?

victoryNap · 2018-06-01T14:40:35Z

@gmlewis I'll take a look. Probably a result of my indentation changes

victoryNap · 2018-06-02T16:02:09Z

@gmlewis Fixed :)

@shurcooL Mind taking a look when you get a chance?

victoryNap · 2018-06-20T16:27:10Z

@gmlewis who else can approve this PR before it can be merged?

gmlewis · 2018-06-21T03:11:20Z

I don't believe that there is any strict rule for code reviewers for this repo... we have typically relied on active contributors but I know we lost some people when the repo required two-factor auth to be enabled, unfortunately.

For non-trivial changes, we like to have a second set of eyes review the code and check that things look good with respect to the GitHub Developer API docs.

So I believe we welcome all contributors to check for the NeedsReview label and provide any feedback, including full reviews and LGTMs.

@willnorris can correct me if my understanding is incorrect.

majormoses · 2018-07-09T21:25:07Z

I will try to crawl through but golang is not exactly something I am comfortable with. One thing that I noticed in the documentation is that if the object is null then that means turning it off.

https://developer.github.com/v3/repos/branches/#parameters-1

EDIT: looking through the tests it looks like its covered.

victoryNap · 2018-07-23T23:59:51Z

@willnorris @dmitshur Can I get another review on this?

victoryNap · 2018-08-01T17:45:05Z

@gmlewis I really need this feature added in. Any thoughts on getting another review?

gmlewis · 2018-08-01T22:12:17Z

Sorry about that... Let's see if @juliaferraioli has time.

gmlewis · 2018-08-10T17:11:17Z

I'm hearing no objections.
Merging.

…ogle#880)

gmlewis · 2018-08-10T18:55:44Z

Fixes #876.

tadityar · 2018-10-08T09:14:05Z

I think this also Fixes #905.

…ogle#880)

victoryNap added 2 commits March 27, 2018 19:11

Added RequiredApprovingReviewCount to PullRequestReviewsEnforcement

a49ec22

Added tests

c51d8bb

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Mar 28, 2018

gmlewis changed the title ~~Added RequiredApprovingReviewCount to PullRequestReviewsEnforcement~~ Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement Mar 28, 2018

victoryNap added 2 commits March 28, 2018 09:18

Fixed tests

d578d31

Merge branch 'master' into master

70d6dcf

victoryNap added 2 commits March 29, 2018 11:56

Added tags and made associated values pointers, also updated related …

df5d65e

…tests

Updated documentation to specify when values are optional

ef58a35

dmitshur reviewed Mar 30, 2018

View reviewed changes

victoryNap mentioned this pull request May 1, 2018

[Feature Request] Support Branch protection for setting required_approving_review_count #905

Closed

victoryNap added 5 commits May 3, 2018 20:05

Added a new header for RequiredApprovingReviewsPreview, replaced and …

5af409a

…removed old repo protection header. Updated tests with new headers and valid values

Merge branch 'master' into master

4e70bd3

Fixed formatting on repos_test.go

da53bef

Merge branch 'master' of https://github.com/victoryNap/go-github

2b2e379

Updated formatting on github.go

1cba8d0

gmlewis reviewed May 11, 2018

View reviewed changes

gmlewis requested changes May 11, 2018

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label May 11, 2018

Updated comment to reflect that new field is not optional

485a5d5

gmlewis reviewed May 31, 2018

View reviewed changes

Fix indentation.

09a561d

gmlewis requested changes Jun 1, 2018

View reviewed changes

Removed unnecessary omitempty annotations.

0966f88

gmlewis approved these changes Jun 1, 2018

View reviewed changes

Fixed tests to include new value

7aff404

gmlewis requested a review from juliaferraioli August 1, 2018 22:12

This was referenced Aug 9, 2018

[Feature Request] support requiring "signed commits" on github_branch_protection integrations/terraform-provider-github#87

Closed

[Feature Request] support required_approving_review_count integrations/terraform-provider-github#89

Closed

gmlewis merged commit 8292762 into google:master Aug 10, 2018

gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement (go…

6cb9338

…ogle#880)

gmlewis mentioned this pull request Aug 10, 2018

Support preview Protected Branches API #876

Closed

acc-tests mentioned this pull request Oct 8, 2018

Add required_approving_review_count #89 (required go-github update) integrations/terraform-provider-github#159

Closed

quantumew mentioned this pull request Oct 8, 2018

Add required_approving_review_count #89 (required go-github update) integrations/terraform-provider-github#160

Closed

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement (go…

ab86bd0

…ogle#880)

gmlewis mentioned this pull request Jan 17, 2021

Remove mediaTypeRequiredApprovingReviewsPreview header from remaining APIs #1779

Closed

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Mar 8, 2023

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement #880

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement #880

Conversation

victoryNap commented Mar 28, 2018

gmlewis commented Mar 28, 2018

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

victoryNap commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoryNap commented May 30, 2018 • edited Loading

gmlewis commented May 31, 2018

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoryNap commented Jun 1, 2018

gmlewis commented Jun 1, 2018

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented Jun 1, 2018

victoryNap commented Jun 1, 2018

victoryNap commented Jun 2, 2018

victoryNap commented Jun 20, 2018 • edited Loading

gmlewis commented Jun 21, 2018

majormoses commented Jul 9, 2018 • edited Loading

victoryNap commented Jul 23, 2018

victoryNap commented Aug 1, 2018

gmlewis commented Aug 1, 2018

gmlewis commented Aug 10, 2018

gmlewis commented Aug 10, 2018

tadityar commented Oct 8, 2018

dmitshur Mar 30, 2018 •

edited

Loading

victoryNap commented May 30, 2018 •

edited

Loading

victoryNap commented Jun 20, 2018 •

edited

Loading

majormoses commented Jul 9, 2018 •

edited

Loading