Skip to content
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

Adds RequireCodeOwnerReviews to PullRequestReviewsEnforcement #744

Merged
merged 2 commits into from
Oct 14, 2017
Merged

Adds RequireCodeOwnerReviews to PullRequestReviewsEnforcement #744

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12f91bc
Adds RequireCodeOwnerReviews to PullRequestReviewsEnforcement
Oct 5, 2017
14a81fd
updating comments to reflect repo's style
Oct 14, 2017
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
10 changes: 10 additions & 0 deletions github/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,8 @@ type PullRequestReviewsEnforcement struct {
DismissalRestrictions DismissalRestrictions `json:"dismissal_restrictions"`
// Specifies if approved reviews are dismissed automatically, when a new commit is pushed.
DismissStaleReviews bool `json:"dismiss_stale_reviews"`
// Specifies if an approved review is required in pull requests including files with a designated code owner.
Copy link
Contributor

@elliott-beach elliott-beach Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

To maintain the same style as the rest of the codebase, the comment should begin with
// RequireCodeOwnerReviews specifies...,
which is also in keeping with standard Go doc-comments.

Copy link
Member

@dmitshur dmitshur Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(That is a hard requirement for top level identifiers, but for struct fields, I think both ways are acceptable. This was consistent with the documentation for fields above.)

RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
}

// PullRequestReviewsEnforcementRequest represents request to set the pull request review
Expand All @@ -572,6 +574,8 @@ type PullRequestReviewsEnforcementRequest struct {
DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"`
// Specifies if approved reviews can be dismissed automatically, when a new commit is pushed. (Required)
DismissStaleReviews bool `json:"dismiss_stale_reviews"`
// Specifies if an approved review is required in pull requests including files with a designated code owner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
}

// MarshalJSON implements the json.Marshaler interface.
Expand All @@ -581,18 +585,22 @@ func (req PullRequestReviewsEnforcementRequest) MarshalJSON() ([]byte, error) {
newReq := struct {
R []interface{} `json:"dismissal_restrictions"`
D bool `json:"dismiss_stale_reviews"`
O bool `json:"require_code_owner_reviews"`
}{
R: []interface{}{},
D: req.DismissStaleReviews,
O: req.RequireCodeOwnerReviews,
}
return json.Marshal(newReq)
}
newReq := struct {
R *DismissalRestrictionsRequest `json:"dismissal_restrictions"`
D bool `json:"dismiss_stale_reviews"`
O bool `json:"require_code_owner_reviews"`
}{
R: req.DismissalRestrictionsRequest,
D: req.DismissStaleReviews,
O: req.RequireCodeOwnerReviews,
}
return json.Marshal(newReq)
}
Expand All @@ -605,6 +613,8 @@ type PullRequestReviewsEnforcementUpdate struct {
DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions,omitempty"`
// Specifies if approved reviews can be dismissed automatically, when a new commit is pushed. Can be omitted.
DismissStaleReviews *bool `json:"dismiss_stale_reviews,omitempty"`
// 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"`
}

// AdminEnforcement represents the configuration to enforce required status checks for repository administrators.
Expand Down
17 changes: 11 additions & 6 deletions github/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,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},"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"}]}}`)
})

protection, _, err := client.Repositories.GetBranchProtection(context.Background(), "o", "r", "b")
Expand All @@ -541,6 +541,7 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {
{Slug: String("t"), ID: Int(4)},
},
},
RequireCodeOwnerReviews: true,
},
EnforceAdmins: &AdminEnforcement{
URL: String("/repos/o/r/branches/b/protection/enforce_admins"),
Expand Down Expand Up @@ -591,7 +592,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":{"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},"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":"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"}]}}`)
})

protection, _, err := client.Repositories.UpdateBranchProtection(context.Background(), "o", "r", "b", input)
Expand All @@ -614,6 +615,7 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) {
{Slug: String("tt"), ID: Int(4)},
},
},
RequireCodeOwnerReviews: true,
},
Restrictions: &BranchRestrictions{
Users: []*User{
Expand Down Expand Up @@ -739,7 +741,7 @@ 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}`)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true}`)
})

enforcement, _, err := client.Repositories.GetPullRequestReviewEnforcement(context.Background(), "o", "r", "b")
Expand All @@ -757,6 +759,7 @@ func TestRepositoriesService_GetPullRequestReviewEnforcement(t *testing.T) {
{Slug: String("t"), ID: Int(2)},
},
},
RequireCodeOwnerReviews: true,
}

if !reflect.DeepEqual(enforcement, want) {
Expand Down Expand Up @@ -784,7 +787,7 @@ func TestRepositoriesService_UpdatePullRequestReviewEnforcement(t *testing.T) {
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}`)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true}`)
})

enforcement, _, err := client.Repositories.UpdatePullRequestReviewEnforcement(context.Background(), "o", "r", "b", input)
Expand All @@ -802,6 +805,7 @@ func TestRepositoriesService_UpdatePullRequestReviewEnforcement(t *testing.T) {
{Slug: String("t"), ID: Int(2)},
},
},
RequireCodeOwnerReviews: true,
}
if !reflect.DeepEqual(enforcement, want) {
t.Errorf("Repositories.UpdatePullRequestReviewEnforcement returned %+v, want %+v", enforcement, want)
Expand All @@ -816,7 +820,7 @@ func TestRepositoriesService_DisableDismissalRestrictions(t *testing.T) {
testMethod(t, r, "PATCH")
testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview)
testBody(t, r, `{"dismissal_restrictions":[]}`+"\n")
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[],"teams":[]},"dismiss_stale_reviews":true}`)
fmt.Fprintf(w, `{"dismissal_restrictions":{"users":[],"teams":[]},"dismiss_stale_reviews":true,"require_code_owner_reviews":true}`)
})

enforcement, _, err := client.Repositories.DisableDismissalRestrictions(context.Background(), "o", "r", "b")
Expand All @@ -830,6 +834,7 @@ func TestRepositoriesService_DisableDismissalRestrictions(t *testing.T) {
Users: []*User{},
Teams: []*Team{},
},
RequireCodeOwnerReviews: true,
}
if !reflect.DeepEqual(enforcement, want) {
t.Errorf("Repositories.DisableDismissalRestrictions returned %+v, want %+v", enforcement, want)
Expand Down Expand Up @@ -925,7 +930,7 @@ func TestPullRequestReviewsEnforcementRequest_MarshalJSON_nilDismissalRestirctio
t.Errorf("PullRequestReviewsEnforcementRequest.MarshalJSON returned error: %v", err)
}

want := `{"dismissal_restrictions":[],"dismiss_stale_reviews":false}`
want := `{"dismissal_restrictions":[],"dismiss_stale_reviews":false,"require_code_owner_reviews":false}`
if want != string(json) {
t.Errorf("PullRequestReviewsEnforcementRequest.MarshalJSON returned %+v, want %+v", string(json), want)
}
Expand Down