[Issue#617] Update Protected Branches API preview #637

jkosecki · 2017-05-10T23:11:24Z

Adds pull request reviews enforcement structures and API methods, adds admin enforcement API methods and removes old IncludeAdmins field from the request.

dmitshur

This is a pretty large change. Thanks for working on this.

I've reviewed it partially so far, and everything looks great. I posted some comments inline.

My biggest concern here is the new PullRequestReviewsEnforcementPatchRequest struct. I think it's unfortunate we need that in addition to PullRequestReviewsEnforcementRequest. I understand why you added it. It's because GitHub API docs currently show slightly different things for these endpoints:

The former says that they're all required fields:

But the latter says they're optional:

First of all, the first one lists dismissal_restrictions object twice, that's clearly a typo. But I wonder if they're not actually that different, maybe the difference is a typo too. It might be good to clarify with GitHub support.

One last thing. I noticed in the docs:

You can disable dismissal restrictions by passing the dismissal_restrictions object as an empty array.

Is there support for that in this PR?

dmitshur · 2017-05-12T02:02:24Z

github/repos.go

+// because the request structure is different from the response structure.
+type PullRequestReviewsEnforcementRequest struct {
+	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"`
+	DismissStaleReviews          bool                          `json:"dismiss_stale_reviews"`


Can you document the two fields?

// Specify which users and teams can dismiss pull request reviews. (Required.) DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"` // Dismiss approved reviews automatically when a new commit is pushed. (Required.) DismissStaleReviews bool `json:"dismiss_stale_reviews"`

Will do and had so at the beginning, but I've noticed that Visual Studio Code go fmt plugin, does not align structure's fields to one line, if the fields have comments. So I thought that maybe commenting fields is not welcome.

They're certainly welcome. See some other structs in this file that have documented fields. The gofmt formatting is fine. Thanks.

dmitshur · 2017-05-12T02:16:05Z

github/repos.go

+// because the patch request does not require all fields to be initialized.
+type PullRequestReviewsEnforcementPatchRequest struct {
+	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions,omitempty"`
+	DismissStaleReviews          *bool                         `json:"dismiss_stale_reviews,omitempty"`


Here as well. Except, the fields are not required here.

dmitshur · 2017-05-12T02:17:08Z

github/repos.go

+// enforcement of a protected branch. It is separate from PullRequestReviewsEnforcement above
+// because the request structure is different from the response structure.
+type PullRequestReviewsEnforcementRequest struct {
+	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"`


Given this is a required field here, what do you think about using a value DismissalRestrictionsRequest rather than pointer *DismissalRestrictionsRequest (similar to bool below being a value rather than pointer).

Or is it neccessary to have a pointer to be able to send a null value?

True, I believe I overthought it.

You can disable dismissal restrictions by passing the dismissal_restrictions object as an empty array.

I will clarify it yet with GitHub support, since other structures allow null for disabling:

You must pass the following objects: required_status_checks, enforce_admins, and restrictions. They can have the value null for disabled.

In the case there is also an type error and empty array should by null object, the pointer would make sense and it would support disabling dismissal restrictions. If not, I'm not sure yet, how can I pass an empty array instead of empty object to disable it.

dmitshur · 2017-05-12T02:25:14Z

github/repos.go

+// PullRequestReviewsEnforcementPatchRequest represents request to patch the pull request review
+// enforcement of a protected branch. It is separate from PullRequestReviewsEnforcementPatchRequest above
+// because the patch request does not require all fields to be initialized.
+type PullRequestReviewsEnforcementPatchRequest struct {


If it turns out we really do need to keep this struct after all, I'd suggest renaming it to PullRequestReviewsEnforcementUpdate. How's that sound?

I'm fine with that. However, taking into account, that the HTTP request is PATCH, I thought it will be better to use such a name, so it will explicitly say, that not all fields are required. But I believe PullRequestReviewsEnforcementUpdate is also fine

The HTTP method is indeed PATCH, but that's a low level implementation detail. The endpoint is called "Update pull request review enforcement of protected branch", which is where my suggestion of using "Update" comes from.

It's still not ideal, because "PullRequestReviewsEnforcementRequest" is used for an endpoint called "Update branch protection"... But still, I think one word is better than "PatchRequest".

jkosecki · 2017-05-12T07:50:40Z

I believe the difference between must and can comes from the difference between HTTP PUT request of the Update branch protection and HTTP PATCH request of the Update pull request reviews enforcment.

I wrote an e-mail to GitHub support to clarify those two endpoints

jkosecki · 2017-05-25T08:51:51Z

@shurcooL:
After exchanging some e-mails with a GitHub support, I know, that we require two different structures for Update branch protection and Update pull request review. The difference between required and not required is intended.

However, I'm still stuck at one point. For update branch protection, it is easy to say "if one pass nil value for dismissal_restrictions, he wants to disable them so we convert it to an empty array.
However, the same option should be available with the Update pull request review and here comes my design problem, because on the one hand we should be able to ignore dismissal_restrictions (that's why omitempty), but on the other hand, we should be able to notify GitHub that one wants to disable them and convert it to an empty array.

dmitshur · 2017-05-25T09:05:42Z

The problem you're describing sounds very similar to #236.

jkosecki · 2017-05-27T09:10:02Z

@shurcooL: Thank you. I took your advise and implemented additional API method. I believe, my change set is ready.

dmitshur

I've left some minor comments.

I'm finding it hard to review the PR as a whole, because of its large size. But the changes you've made seem reasonable.

@gmlewis Can you take a look and see if you have any comments?

dmitshur · 2017-06-02T20:01:18Z

github/repos.go

+		D: req.DismissStaleReviews,
+	}
+	return json.Marshal(newReq)
+}


This is a really cool solution to work around the GitHub API weirdness. Nice idea! 👍

dmitshur · 2017-06-02T20:03:10Z

github/repos_test.go

+		body := strings.TrimSpace(buf.String())
+		if wantBody != body {
+			t.Errorf("Request body = %+v, want %+v", body, wantBody)
+		}


You can use testBody helper. See how other tests use it.

Thank you for the tip, it made the test method definitely nicer.

dmitshur · 2017-06-02T20:06:03Z

github/repos_test.go

+	}
+
+	want := `{"dismissal_restrictions":[],"dismiss_stale_reviews":false}`
+


This blank line can be removed too.

dmitshur · 2017-06-02T20:09:32Z

github/repos_test.go

+
+	json, err := json.Marshal(req)
+
+	if err != nil {


Remove the blank line between foo, err := something() and if err != nil {. It hurts readability.

gmlewis

LGTM after @shurcooL's comments and a minor comment typo are fixed.
Thanks, @navarrano!

gmlewis · 2017-06-02T20:37:37Z

github/repos.go

+}
+
+// PullRequestReviewsEnforcementUpdate represents request to patch the pull request review
+// enforcement of a protected branch. It is separate from PullRequestReviewsEnforcementUpdate above


s/PullRequestReviewsEnforcementUpdate/PullRequestReviewsEnforcementRequest/

@gmlewis The reason it's not already PullRequestReviewsEnforcementRequest is because that name is already taken. See 3rd paragraph of #637 (review) and #637 (review) for previous discussion about this.

Let X = "PullRequestReviewsEnforcementUpdate"

This section says:

// X represents... It is separate from X above ...
type X struct {

I think the second X needs to be replaced. The first and third X are fine.

Ah, I see what you mean now. 👍 Indeed.

dmitshur · 2017-06-02T21:11:27Z

github/repos.go

+//
+// GitHub API docs: https://developer.github.com/v3/repos/branches/#get-pull-request-review-enforcement-of-protected-branch
+func (s *RepositoriesService) GetPullRequestReviewEnforcement(ctx context.Context, owner, repo, branch string) (*PullRequestReviewsEnforcement, *Response, error) {
+	u := fmt.Sprintf("/repos/%v/%v/branches/%v/protection/required_pull_request_reviews", owner, repo, branch)


An important issue I've spotted is that all 7 of these URLs begin with a /, they should not (it breaks support for GitHub Enterprise). Instead, write:

u := fmt.Sprintf("repos/...

Thank you for spotting it, no idea how I came with an idea of starting the URL with a slash...
Should be alright now

dmitshur

I'm not spotting any other issues. LGTM.

I haven't verified absolutely all details in this PR (because of its size), but it looks good. I'm sure we can address any issues if they come up. This is still a preview API.

Thanks!

dmitshur · 2017-06-03T05:46:46Z

github/repos.go

+	return r, resp, err
+}
+
+// RemoveAdminEnforcement removess admin enforcement from a protected branch.


Minor, typo in "removess", should be "removes".

gmlewis · 2017-06-28T21:49:28Z

Sorry for the delay! LGTM. Merging.

Updates google#617.

jkosecki added 3 commits May 11, 2017 00:13

[google#617] Adds pull request reviews enforcement handling

84e6b44

[google#617] Removes IncludeAdmins

6f90ac1

[google#617] Adds AdminEnforcement API methods

3604ffb

dmitshur self-requested a review May 11, 2017 04:24

dmitshur reviewed May 12, 2017

View reviewed changes

dmitshur added the waiting for GitHub label May 12, 2017

Adds suggested pull request changes

03116cd

Adds disable dismissal restrictions API method

f1e6a0d

dmitshur removed the waiting for GitHub label May 28, 2017

dmitshur requested changes Jun 2, 2017

View reviewed changes

dmitshur reviewed Jun 2, 2017

View reviewed changes

dmitshur requested a review from gmlewis June 2, 2017 20:10

gmlewis requested changes Jun 2, 2017

View reviewed changes

dmitshur requested changes Jun 2, 2017

View reviewed changes

Removes redudant initial slash from requests repos. Adds small fixes

9cc75dc

dmitshur approved these changes Jun 3, 2017

View reviewed changes

Fixes typo in RemoveAdminEnforcement documentation

01a20ed

grubernaut mentioned this pull request Jun 28, 2017

Support enforce_admins API schema in Github Branch Protection integrations/terraform-provider-github#18

Closed

gmlewis approved these changes Jun 28, 2017

View reviewed changes

gmlewis merged commit b5b6d4c into google:master Jun 28, 2017

gmlewis mentioned this pull request Jun 28, 2017

Update Protected Branches API preview #617

Closed

jkosecki deleted the issue#617 branch June 29, 2017 10:46

dmitshur mentioned this pull request Aug 22, 2017

Fix #664. Docs for empty DismissalRestrictions were incorrect. [] -> {} #703

Closed

elliott-beach mentioned this pull request Oct 13, 2017

Adds RequireCodeOwnerReviews to PullRequestReviewsEnforcement #744

Merged

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

[Issue#617] Update Protected Branches API preview (google#637)

a82469c

Updates google#617.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue#617] Update Protected Branches API preview #637

[Issue#617] Update Protected Branches API preview #637

jkosecki commented May 10, 2017

dmitshur left a comment

dmitshur May 12, 2017

jkosecki May 12, 2017

dmitshur May 12, 2017

dmitshur May 12, 2017

dmitshur May 12, 2017

jkosecki May 12, 2017 •

edited

Loading

dmitshur May 12, 2017

jkosecki May 12, 2017

dmitshur May 12, 2017 •

edited

Loading

jkosecki commented May 12, 2017

jkosecki commented May 25, 2017

dmitshur commented May 25, 2017

jkosecki commented May 27, 2017

dmitshur left a comment

dmitshur Jun 2, 2017

dmitshur Jun 2, 2017

jkosecki Jun 2, 2017

dmitshur Jun 2, 2017

dmitshur Jun 2, 2017 •

edited

Loading

gmlewis left a comment

gmlewis Jun 2, 2017

dmitshur Jun 2, 2017

gmlewis Jun 2, 2017

dmitshur Jun 2, 2017

dmitshur Jun 2, 2017 •

edited

Loading

jkosecki Jun 2, 2017

dmitshur left a comment •

edited

Loading

dmitshur Jun 3, 2017

gmlewis commented Jun 28, 2017

		}

		want := `{"dismissal_restrictions":[],"dismiss_stale_reviews":false}`

[Issue#617] Update Protected Branches API preview #637

[Issue#617] Update Protected Branches API preview #637

Conversation

jkosecki commented May 10, 2017

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

jkosecki May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur May 12, 2017 • edited Loading

Choose a reason for hiding this comment

jkosecki commented May 12, 2017

jkosecki commented May 25, 2017

dmitshur commented May 25, 2017

jkosecki commented May 27, 2017

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

dmitshur Jun 2, 2017 • edited Loading

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

dmitshur Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented Jun 28, 2017

jkosecki May 12, 2017 •

edited

Loading

dmitshur May 12, 2017 •

edited

Loading

dmitshur Jun 2, 2017 •

edited

Loading

dmitshur Jun 2, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading