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.

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

merged 2 commits into from
Oct 14, 2017

Conversation

denniswebb
Copy link
Contributor

While working on terraform-providers/terraform-provider-github#50 I realized that this was missing.

GitHub.com support page about feature https://developer.github.com/v3/repos/branches/#update-branch-protection

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Oct 5, 2017
@elliott-beach
Copy link
Contributor

Bump.

The discrepancy came up as a result of changes in #637. As @shurcooL mentioned, we hadn't fully reviewed all the code in that pull request, so it's natural for this to occur.

I believe this might also fix #664, since that has to do with the PullRequestReviewsEnforcementRequest struct which is fixed here by adding a required parameter.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

LGTM.
Awaiting second LGTM before merging.
Thank you, @denniswebb and @elliott-beach!

@gmlewis gmlewis requested a review from elliott-beach October 13, 2017 21:08
@elliott-beach
Copy link
Contributor

It feels somewhat despotic to have the power to review PRs, but will proceed to do so, @gmlewis.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 14, 2017

Hi Elliott, I apologize for not discussing this with you first... are you open to reviewing PRs for this repo? You come highly recommended. 😄
Also, just FYI, I'm about to go on vacation for 9 days and may not be very active on this repo during this time period. Others may step in to fill the gap, but if not, I'll try to catch up when I return.

github/repos.go Outdated
@@ -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

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

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.)

github/repos.go Outdated
@@ -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.

Copy link
Contributor

@elliott-beach elliott-beach left a comment

Choose a reason for hiding this comment

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

Minor nit, otherwise LGTM.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 14, 2017

Thank you!
Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants