-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add new branch protection values #1393
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @tjamet.
This is a good start, but please address the comments below and we can then proceed with the PR.
github/repos.go
Outdated
@@ -735,6 +738,12 @@ type ProtectionRequest struct { | |||
RequiredPullRequestReviews *PullRequestReviewsEnforcementRequest `json:"required_pull_request_reviews"` | |||
EnforceAdmins bool `json:"enforce_admins"` | |||
Restrictions *BranchRestrictionsRequest `json:"restrictions"` | |||
// Enforces a linear commit Git history, which prevents anyone from pushing merge commits to a branch. | |||
RequireLinearHistory *bool `json:"required_linear_history,omiteempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omitempty
is spelled incorrectly each time. Please fix all three.
github/repos.go
Outdated
@@ -735,6 +738,12 @@ type ProtectionRequest struct { | |||
RequiredPullRequestReviews *PullRequestReviewsEnforcementRequest `json:"required_pull_request_reviews"` | |||
EnforceAdmins bool `json:"enforce_admins"` | |||
Restrictions *BranchRestrictionsRequest `json:"restrictions"` | |||
// Enforces a linear commit Git history, which prevents anyone from pushing merge commits to a branch. | |||
RequireLinearHistory *bool `json:"required_linear_history,omiteempty"` | |||
// Permits force pushes to the protected branch by anyone with write access to the repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every new comment in this PR needs to be a complete sentence ending in a period (".") so that the auto-generated Godocs will read nicely, and to be consistent with the rest of the repo. Please check each comment and fix as needed.
github/repos.go
Outdated
@@ -727,6 +727,9 @@ type Protection struct { | |||
RequiredPullRequestReviews *PullRequestReviewsEnforcement `json:"required_pull_request_reviews"` | |||
EnforceAdmins *AdminEnforcement `json:"enforce_admins"` | |||
Restrictions *BranchRestrictions `json:"restrictions"` | |||
RequireLinearHistory *LinearHistoryEnforcement `json:"required_linear_history"` | |||
AllowForcePushes *ForcePushTolerance `json:"allow_force_pushes"` | |||
AllowDeletions *DeletionTolerance `json:"allow_deletions"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although these three new struct names are good choices on their own, let's please rename them to match the name of the field, for two reasons... first, to be more consistent with the rest of the repo, and second to reduce confusion when searching for the struct or the field. So to be clear, let's please rename these structs to RequireLinearHistory
, AllowForcePushes
, and AllowDeletions
, respectively.
GitHub has been adding some new fields in the branch protections as documented: https://developer.github.com/v3/repos/branches/#get-branch-protection
Run go generate
371e8b8
to
0f042d8
Compare
Thanks for the fast feedback, request changes addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @tjamet. LGTM. Merging.
(In the future, please try to remember not to force push commits to PRs for this repo, as it makes it more difficult for reviewers to see what changed since the last code review... not a big deal in this case, but if you make bigger PRs in the future, it is tremendously appreciated. Thanks. 😄)
GitHub has been adding some new fields in the branch protections
as documented:
https://developer.github.com/v3/repos/branches/#get-branch-protection
the documentation references the new fields in the branch protection as:
https://developer.github.com/v3/repos/branches/#get-branch-protection:
GET /repos/:owner/:repo/branches/:branch/protection
https://developer.github.com/v3/repos/branches/#update-branch-protection:
PUT /repos/:owner/:repo/branches/:branch/protection
required_linear_history
allow_force_pushes
allow_deletions
This PR achieves a similar goal as #1354, addressing comments.
Unlike suggestion in #1354 comment, the
EnabledType
is not defined as is, instead individual types are declared to prevent interface change if ever the field type changes.Fixes #1345