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

Add support for new fields in Branch Protection #1354

Closed
wants to merge 1 commit into from

Conversation

pcantea
Copy link

@pcantea pcantea commented Dec 23, 2019

Fixes #1345

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Dec 23, 2019
Copy link
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

Thank you, @pcantea
👌

Awaiting second LGTM before merging.

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.

Sorry, @pcantea, but this PR will not work as written. See details below.

@@ -728,6 +728,9 @@ type Protection struct {
RequiredPullRequestReviews *PullRequestReviewsEnforcement `json:"required_pull_request_reviews"`
EnforceAdmins *AdminEnforcement `json:"enforce_admins"`
Restrictions *BranchRestrictions `json:"restrictions"`
RequiredLinearHistory bool `json:"required_linear_history"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each of these three bool fields need to be structures with a single Enabled field, based on the GitHub v3 API developer docs found here:
https://developer.github.com/v3/repos/branches/#get-branch-protection

Here is the example output from this endpoint:

...
  "required_linear_history": {
    "enabled": true
  },
  "allow_force_pushes": {
    "enabled": true
  },
  "allow_deletions": {
    "enabled": true
  }

Since each of these structs appear to only have a single Enabled field, I'm fine with making them all the same struct type... something like:

type EnabledType struct {
  Enabled bool `json:"enabled"`
}

and then these three fields would have struct pointers with omitempty, like this:

RequiredLinearHistory *EnabledType `json:"required_linear_history,omitempty"`
AllowForcePushes      *EnabledType `json:"allow_force_pushes,omitempty"`
AllowDeletions        *EnabledType `json:"allow_deletions,omitempty"`

That way, if GitHub provides the values, great... but if not, we know that they weren't returned.

@@ -736,6 +739,9 @@ type ProtectionRequest struct {
RequiredPullRequestReviews *PullRequestReviewsEnforcementRequest `json:"required_pull_request_reviews"`
EnforceAdmins bool `json:"enforce_admins"`
Restrictions *BranchRestrictionsRequest `json:"restrictions"`
RequiredLinearHistory bool `json:"required_linear_history"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, this won't work either. Because these are optional fields, these three need to be type *bool and not bool.
They also need the ,omiteempty flag on them

Copy link
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

Sorry for my last review, I was a little bit confused about the docs. My bad

@gmlewis gmlewis closed this in 8f818bc Jan 24, 2020
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
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.

Feature Request > New branch protection features support
4 participants