From 1474de2934ec3667d1d69b394a0079a794a881f0 Mon Sep 17 00:00:00 2001 From: Andy Lindeman Date: Sun, 8 Jan 2017 21:30:11 -0500 Subject: [PATCH 1/4] Adds required pull request reviews `required_pull_request_reviews` is a newly introduced object: * https://developer.github.com/v3/repos/branches/#get-branch-protection * https://developer.github.com/v3/repos/branches/#update-branch-protection --- github/repos.go | 16 ++++++++++++---- github/repos_test.go | 13 +++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/github/repos.go b/github/repos.go index 040cd31e006..5af8883db30 100644 --- a/github/repos.go +++ b/github/repos.go @@ -508,14 +508,16 @@ type Branch struct { // Protection represents a repository branch's protection. type Protection struct { - RequiredStatusChecks *RequiredStatusChecks `json:"required_status_checks"` - Restrictions *BranchRestrictions `json:"restrictions"` + RequiredStatusChecks *RequiredStatusChecks `json:"required_status_checks"` + RequiredPullRequestReviews *RequiredPullRequestReviews `json:"required_pull_request_reviews"` + Restrictions *BranchRestrictions `json:"restrictions"` } // ProtectionRequest represents a request to create/edit a branch's protection. type ProtectionRequest struct { - RequiredStatusChecks *RequiredStatusChecks `json:"required_status_checks"` - Restrictions *BranchRestrictionsRequest `json:"restrictions"` + RequiredStatusChecks *RequiredStatusChecks `json:"required_status_checks"` + RequiredPullRequestReviews *RequiredPullRequestReviews `json:"required_pull_request_reviews"` + Restrictions *BranchRestrictionsRequest `json:"restrictions"` } // RequiredStatusChecks represents the protection status of a individual branch. @@ -529,6 +531,12 @@ type RequiredStatusChecks struct { Contexts *[]string `json:"contexts,omitempty"` } +// RequiredPullRequestReviews represents the protection configuration for pull requests. +type RequiredPullRequestReviews struct { + // Enforce pull request reviews for repository administrators. + IncludeAdmins *bool `json:"include_admins,omitempty"` +} + // BranchRestrictions represents the restriction that only certain users or // teams may push to a branch. type BranchRestrictions struct { diff --git a/github/repos_test.go b/github/repos_test.go index 20598d03fec..4304ab8c8d3 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -481,7 +481,7 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { testMethod(t, r, "GET") testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview) - fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`) + fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`) }) protection, _, err := client.Repositories.GetBranchProtection("o", "r", "b") @@ -495,6 +495,9 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { Strict: Bool(true), Contexts: &[]string{"continuous-integration"}, }, + RequiredPullRequestReviews: &RequiredPullRequestReviews{ + IncludeAdmins: Bool(true), + }, Restrictions: &BranchRestrictions{ Users: []*User{ {Login: String("u"), ID: Int(1)}, @@ -519,6 +522,9 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { Strict: Bool(true), Contexts: &[]string{"continuous-integration"}, }, + RequiredPullRequestReviews: &RequiredPullRequestReviews{ + IncludeAdmins: Bool(true), + }, Restrictions: &BranchRestrictionsRequest{ Users: &[]string{"u"}, Teams: &[]string{"t"}, @@ -534,7 +540,7 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { t.Errorf("Request body = %+v, want %+v", v, input) } testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview) - fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`) + fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`) }) protection, _, err := client.Repositories.UpdateBranchProtection("o", "r", "b", input) @@ -548,6 +554,9 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { Strict: Bool(true), Contexts: &[]string{"continuous-integration"}, }, + RequiredPullRequestReviews: &RequiredPullRequestReviews{ + IncludeAdmins: Bool(true), + }, Restrictions: &BranchRestrictions{ Users: []*User{ {Login: String("u"), ID: Int(1)}, From 939e936130becbc0301d2a122b389e14856cd328 Mon Sep 17 00:00:00 2001 From: Andy Lindeman Date: Tue, 10 Jan 2017 20:37:29 -0500 Subject: [PATCH 2/4] `include_admins` is a required value By making it a value, rather than a pointer to a value, we drive home the point that it must be specified and is always present in responses. --- github/repos.go | 2 +- github/repos_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/github/repos.go b/github/repos.go index 5af8883db30..ea886bd062d 100644 --- a/github/repos.go +++ b/github/repos.go @@ -534,7 +534,7 @@ type RequiredStatusChecks struct { // RequiredPullRequestReviews represents the protection configuration for pull requests. type RequiredPullRequestReviews struct { // Enforce pull request reviews for repository administrators. - IncludeAdmins *bool `json:"include_admins,omitempty"` + IncludeAdmins bool `json:"include_admins"` } // BranchRestrictions represents the restriction that only certain users or diff --git a/github/repos_test.go b/github/repos_test.go index 4304ab8c8d3..b4217506841 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -496,7 +496,7 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { Contexts: &[]string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ - IncludeAdmins: Bool(true), + IncludeAdmins: true, }, Restrictions: &BranchRestrictions{ Users: []*User{ @@ -523,7 +523,7 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { Contexts: &[]string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ - IncludeAdmins: Bool(true), + IncludeAdmins: true, }, Restrictions: &BranchRestrictionsRequest{ Users: &[]string{"u"}, @@ -555,7 +555,7 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { Contexts: &[]string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ - IncludeAdmins: Bool(true), + IncludeAdmins: true, }, Restrictions: &BranchRestrictions{ Users: []*User{ From 2a12ccd21eac584d3f247c61cd4fd94e5ae68926 Mon Sep 17 00:00:00 2001 From: Andy Lindeman Date: Tue, 10 Jan 2017 20:42:08 -0500 Subject: [PATCH 3/4] Aligns other structs with the new convention Required fields should be values, not pointers, because a lack of a value is an invalid specification. --- github/repos.go | 6 +++--- github/repos_test.go | 18 +++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/github/repos.go b/github/repos.go index ea886bd062d..6feff8b1343 100644 --- a/github/repos.go +++ b/github/repos.go @@ -523,12 +523,12 @@ type ProtectionRequest struct { // RequiredStatusChecks represents the protection status of a individual branch. type RequiredStatusChecks struct { // Enforce required status checks for repository administrators. - IncludeAdmins *bool `json:"include_admins,omitempty"` + IncludeAdmins bool `json:"include_admins"` // Require branches to be up to date before merging. - Strict *bool `json:"strict,omitempty"` + Strict bool `json:"strict"` // The list of status checks to require in order to merge into this // branch. - Contexts *[]string `json:"contexts,omitempty"` + Contexts []string `json:"contexts"` } // RequiredPullRequestReviews represents the protection configuration for pull requests. diff --git a/github/repos_test.go b/github/repos_test.go index b4217506841..e2eb4f4ed89 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -491,9 +491,9 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { want := &Protection{ RequiredStatusChecks: &RequiredStatusChecks{ - IncludeAdmins: Bool(true), - Strict: Bool(true), - Contexts: &[]string{"continuous-integration"}, + IncludeAdmins: true, + Strict: true, + Contexts: []string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ IncludeAdmins: true, @@ -518,9 +518,9 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { input := &ProtectionRequest{ RequiredStatusChecks: &RequiredStatusChecks{ - IncludeAdmins: Bool(true), - Strict: Bool(true), - Contexts: &[]string{"continuous-integration"}, + IncludeAdmins: true, + Strict: true, + Contexts: []string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ IncludeAdmins: true, @@ -550,9 +550,9 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { want := &Protection{ RequiredStatusChecks: &RequiredStatusChecks{ - IncludeAdmins: Bool(true), - Strict: Bool(true), - Contexts: &[]string{"continuous-integration"}, + IncludeAdmins: true, + Strict: true, + Contexts: []string{"continuous-integration"}, }, RequiredPullRequestReviews: &RequiredPullRequestReviews{ IncludeAdmins: true, From 6252e7a493e3dcadfdb70ff52f542f663c2dbaee Mon Sep 17 00:00:00 2001 From: Andy Lindeman Date: Thu, 12 Jan 2017 22:07:37 -0500 Subject: [PATCH 4/4] `users` and `teams` are required keys --- github/repos.go | 8 ++++---- github/repos_test.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/github/repos.go b/github/repos.go index 6feff8b1343..a6a02c3b152 100644 --- a/github/repos.go +++ b/github/repos.go @@ -541,9 +541,9 @@ type RequiredPullRequestReviews struct { // teams may push to a branch. type BranchRestrictions struct { // The list of user logins with push access. - Users []*User `json:"users,omitempty"` + Users []*User `json:"users"` // The list of team slugs with push access. - Teams []*Team `json:"teams,omitempty"` + Teams []*Team `json:"teams"` } // BranchRestrictionsRequest represents the request to create/edit the @@ -552,9 +552,9 @@ type BranchRestrictions struct { // different from the response structure. type BranchRestrictionsRequest struct { // The list of user logins with push access. - Users *[]string `json:"users,omitempty"` + Users []string `json:"users"` // The list of team slugs with push access. - Teams *[]string `json:"teams,omitempty"` + Teams []string `json:"teams"` } // ListBranches lists branches for the specified repository. diff --git a/github/repos_test.go b/github/repos_test.go index e2eb4f4ed89..faa9ac679f6 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -526,8 +526,8 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { IncludeAdmins: true, }, Restrictions: &BranchRestrictionsRequest{ - Users: &[]string{"u"}, - Teams: &[]string{"t"}, + Users: []string{"u"}, + Teams: []string{"t"}, }, }