Skip to content

Support preview Protected Branches API #934

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
+36 −19
Closed

Support preview Protected Branches API #934

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b41e2dc
Get Branch Protection
scriptonist Jun 22, 2018
f867dc6
Added in missing comment, struct field
scriptonist Jun 22, 2018
58b5a64
Changed the bits for all endpoints and tweaked tests
scriptonist Jun 22, 2018
0120b56
added missing comment to mediatype and ran go generate
scriptonist Jun 22, 2018
e9b2196
Merge branch 'master' into Issue876
gmlewis Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions github/github-accessors.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions github/github.go
View file
Original file line number Diff line number Diff line change
@@ -114,6 +114,9 @@ const (
// https://developer.github.com/changes/2018-02-07-team-discussions-api/
mediaTypeTeamDiscussionsPreview = "application/vnd.github.echo-preview+json"

// https://developer.github.com/changes/2018-03-16-protected-branches-required-approving-reviews/
mediaTypeMultipleApprovingReviewsForProtectedBranches = "application/vnd.github.luke-cage-preview+json"

// https://developer.github.com/changes/2018-05-07-new-checks-api-public-beta/
mediaTypeCheckRunsPreview = "application/vnd.github.antiope-preview+json"

16 changes: 9 additions & 7 deletions github/repos.go
View file
Original file line number Diff line number Diff line change
@@ -563,11 +563,13 @@ type RequiredStatusChecksRequest struct {
// PullRequestReviewsEnforcement represents the pull request reviews enforcement of a protected branch.
type PullRequestReviewsEnforcement struct {
// Specifies which users and teams can dismiss pull request reviews.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written. the new omitempty tags aren't useful because the fields are not pointers and therefore can not be nil.

But looking back at https://github.com/google/go-github/pull/637/files , I'm not seeing why these were not originally made to be pointers with the omitempty tag. Maybe @shurcooL remembers? I read through the comments but it looks like I didn't bring up this point before and neither did anyone else.

Unless @shurcooL says otherwise, I'm thinking that these fields need to be turned into pointers and the omitempty tags should be added since these are return values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I didn't get you. Should I make any changes or are we waiting for @shurcooL for a response?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead and change these fields to pointers and add omitempty since this struct is part of the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlewis let me make sure I got It correct, you are suggesting that we add omitempty to the following structs, Right?

https://github.com/scriptonist/go-github/blob/0120b56d9901fcafe451c9d08ea8f446478ce0d0/github/repos.go#L536

https://github.com/scriptonist/go-github/blob/0120b56d9901fcafe451c9d08ea8f446478ce0d0/github/repos.go#L544

https://github.com/scriptonist/go-github/blob/0120b56d9901fcafe451c9d08ea8f446478ce0d0/github/repos.go#L552

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion. It looks like you have already added omitempty to all the PullRequestReviewsEnforcement struct fields, which is what I was requesting.

I'm thinking that to make the omitempty tags effective, we should also make all the fields pointers.

So they would be:

DismissalRestrictions *DismissalRestrictions ...
DismissStaleReviews *bool ...
RequireCodeOwnerReviews *bool ...
...

Please let me know if that is not clear.

@jkosecki - maybe you can comment on what you think about these proposed changes since you authored #617?

Copy link
Contributor Author

@scriptonist scriptonist Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a noob question but why are we using pointers for types like int and bool? @gmlewis

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an excellent question, @scriptonist, and there have been several very good discussions about it. In a nutshell, it is so that we can tell if the server populated the field that it sent back to us which is especially important for variables like bool where you want to know if you are really getting back a false or if the server didn't send a value at all.

It is actually more important on the request side than the response side, really... which is why they may not be needed at all in this situation. I think I'm contradicting what I said earlier, unfortunately, and apologize. I said "since this struct is part of the return value". That is not good enough reason to make this change, really...

So I'm thinking now, let's hold off on my most recent request to make these all pointers.
Especially if the docs seem to indicate that these fields will always be sent in the reply.

Hopefully @jkosecki or @shurcooL can provide a code review and then we can get this merged when they are also happy with the changes. As this stands, I'm going to give this my LGTM.

Thanks again, @scriptonist, for bearing with me.

Here is a search of some of the previous great discussions about using pointers in this repo:
https://github.com/google/go-github/issues?utf8=%E2%9C%93&q=is%3Aissue+pointers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gmlewis for the detailed explanation. I was curious to know why things are like this. This was a perfect answer to that. I really appreciate your willingness to assist.

DismissalRestrictions DismissalRestrictions `json:"dismissal_restrictions"`
DismissalRestrictions DismissalRestrictions `json:"dismissal_restrictions,omitempty"`
// Specifies if approved reviews are dismissed automatically, when a new commit is pushed.
DismissStaleReviews bool `json:"dismiss_stale_reviews"`
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"`
RequireCodeOwnerReviews bool `json:"require_code_owner_reviews,omitempty"`
// RequiredApprovingReviewCount specifies the number of reviewers required to approve pull requests, this will be a number between 1 and 6.
RequiredApprovingReviewCount *int `json:"required_approving_review_count,omitempty"`
}

// PullRequestReviewsEnforcementRequest represents request to set the pull request review
@@ -702,7 +704,7 @@ func (s *RepositoriesService) GetBranchProtection(ctx context.Context, owner, re
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)

p := new(Protection)
resp, err := s.client.Do(ctx, req, p)
@@ -767,7 +769,7 @@ func (s *RepositoriesService) UpdateBranchProtection(ctx context.Context, owner,
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)

p := new(Protection)
resp, err := s.client.Do(ctx, req, p)
@@ -843,7 +845,7 @@ func (s *RepositoriesService) GetPullRequestReviewEnforcement(ctx context.Contex
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)

r := new(PullRequestReviewsEnforcement)
resp, err := s.client.Do(ctx, req, r)
@@ -866,7 +868,7 @@ func (s *RepositoriesService) UpdatePullRequestReviewEnforcement(ctx context.Con
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)

r := new(PullRequestReviewsEnforcement)
resp, err := s.client.Do(ctx, req, r)
28 changes: 16 additions & 12 deletions github/repos_test.go
View file
Original file line number Diff line number Diff line change
@@ -530,8 +530,8 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {
json.NewDecoder(r.Body).Decode(v)

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"}]}}`)
testHeader(t, r, "Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)
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,"required_approving_review_count": 2},"enforce_admins":{"url":"/repos/o/r/branches/b/protection/enforce_admins","enabled":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
})

protection, _, err := client.Repositories.GetBranchProtection(context.Background(), "o", "r", "b")
@@ -554,7 +554,8 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {
{Slug: String("t"), ID: Int64(4)},
},
},
RequireCodeOwnerReviews: true,
RequireCodeOwnerReviews: true,
RequiredApprovingReviewCount: Int(2),
},
EnforceAdmins: &AdminEnforcement{
URL: String("/repos/o/r/branches/b/protection/enforce_admins"),
@@ -604,8 +605,8 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) {
if !reflect.DeepEqual(v, input) {
t.Errorf("Request body = %+v, want %+v", v, input)
}
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":"uu"}],"teams":[{"id":4,"slug":"tt"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
testHeader(t, r, "Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)
fmt.Fprintf(w, `{"required_status_checks":{"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"dismissal_restrictions":{"users":[{"id":3,"login":"uu"}],"teams":[{"id":4,"slug":"tt"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count":2},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
})

protection, _, err := client.Repositories.UpdateBranchProtection(context.Background(), "o", "r", "b", input)
@@ -628,7 +629,8 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) {
{Slug: String("tt"), ID: Int64(4)},
},
},
RequireCodeOwnerReviews: true,
RequireCodeOwnerReviews: true,
RequiredApprovingReviewCount: Int(2),
},
Restrictions: &BranchRestrictions{
Users: []*User{
@@ -791,8 +793,8 @@ func TestRepositoriesService_GetPullRequestReviewEnforcement(t *testing.T) {

mux.HandleFunc("/repos/o/r/branches/b/protection/required_pull_request_reviews", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true}`)
testHeader(t, r, "Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count": 2}`)
})

enforcement, _, err := client.Repositories.GetPullRequestReviewEnforcement(context.Background(), "o", "r", "b")
@@ -810,7 +812,8 @@ func TestRepositoriesService_GetPullRequestReviewEnforcement(t *testing.T) {
{Slug: String("t"), ID: Int64(2)},
},
},
RequireCodeOwnerReviews: true,
RequireCodeOwnerReviews: true,
RequiredApprovingReviewCount: Int(2),
}

if !reflect.DeepEqual(enforcement, want) {
@@ -837,8 +840,8 @@ func TestRepositoriesService_UpdatePullRequestReviewEnforcement(t *testing.T) {
if !reflect.DeepEqual(v, input) {
t.Errorf("Request body = %+v, want %+v", v, input)
}
testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true}`)
testHeader(t, r, "Accept", mediaTypeMultipleApprovingReviewsForProtectedBranches)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true,"required_approving_review_count": 2}`)
})

enforcement, _, err := client.Repositories.UpdatePullRequestReviewEnforcement(context.Background(), "o", "r", "b", input)
@@ -856,7 +859,8 @@ func TestRepositoriesService_UpdatePullRequestReviewEnforcement(t *testing.T) {
{Slug: String("t"), ID: Int64(2)},
},
},
RequireCodeOwnerReviews: true,
RequireCodeOwnerReviews: true,
RequiredApprovingReviewCount: Int(2),
}
if !reflect.DeepEqual(enforcement, want) {
t.Errorf("Repositories.UpdatePullRequestReviewEnforcement returned %+v, want %+v", enforcement, want)