Go-github: The branch protection dismissal restrictions response doesn't distinguish absent from an empty allowed list

[glenjamin]

Follows on from #791

See also https://github.com/terraform-providers/terraform-provider-github/issues/267

With dismissal restrictions disabled the payload looks like this:

  "required_pull_request_reviews": {
    "url": "https://api.github.com/repos/org/project/branches/master/protection/required_pull_request_reviews",
    "dismiss_stale_reviews": true,
    "require_code_owner_reviews": true
  },

With dismissal restrictions enabled, but both lists empty, the payload looks like this:

  "required_pull_request_reviews": {
    "url": "https://api.github.com/repos/circleci/audit-test-restricted-repo/branches/master/protection/required_pull_request_reviews",
    "dismiss_stale_reviews": true,
    "require_code_owner_reviews": true,
    "dismissal_restrictions": {
      "url": "https://api.github.com/repos/org/project/branches/master/protection/dismissal_restrictions",
      "users_url": "https://api.github.com/repos/org/project/branches/master/protection/dismissal_restrictions/users",
      "teams_url": "https://api.github.com/repos/org/project/branches/master/protection/dismissal_restrictions/teams",
      "users": [

      ],
      "teams": [

      ]
    }
  }

However the Struct for this response doesn't model the optionality

// PullRequestReviewsEnforcement represents the pull request reviews enforcement of a protected branch.
type PullRequestReviewsEnforcement struct {
    // Specifies which users and teams can dismiss pull request reviews.
    DismissalRestrictions DismissalRestrictions `json:"dismissal_restrictions"`
    // Specifies if approved reviews are dismissed automatically, when a new commit is pushed.
    DismissStaleReviews bool `json:"dismiss_stale_reviews"`
    // RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
    RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
    // RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
    // Valid values are 1-6.
    RequiredApprovingReviewCount int `json:"required_approving_review_count"`
}

I'm going to look into making a PR for this.

[glenjamin]

For completeness, against the /protection endpoint:

on GET

• if enabled, you get dismissal_restrictions
• if disabled, then dismissal_restrictions is omitted

on PUT

• if dismissal_restrictions is absent, it is unchanged
• if dismissal_restrictions is present, it must be an object
• if the object is empty, dismissal_restrictions are removed
• if the object has teams or users, they must be an array
• including teams and users will update both of them
• if you omit one of teams or users while specifying the other, the omitted one with be emptied

This is the same for the /protection/required_pull_request_reviews endpoint, except the method must be PATCH rather than PUT.

[gmlewis]

From here: https://developer.github.com/v3/repos/branches/#update-pull-request-review-enforcement-of-protected-branch
Since dismissal_restrictions has a definite structure:

why don't we go ahead and make a struct for it with ~optional~ fields for users and teams?

I'm thinking something like this:

type DismisalRestrictions {
  Users []string `json:"users"`
  Teams []string `json:"teams"`
}

and then we can get rid of the []interface{} and the map[string]interface{} entirely.

Thoughts?

Edit: as @glenjamin pointed out below, this struct already exists and is named BranchRestrictionsRequest so we can reuse that... if you think its name should be made more general to fit both scenarios, I'm fine with that... both cases appear to be restrictions, so I don't think this is a bad thing from a design perspective.

[glenjamin]

Good call, that struct already exists for the update payload, so i’ll use that

[gmlewis]

Oh, I think I mis-spoke... they are not optional... they are required. I'll go edit my other comment... and yes, I think you can reuse that already-existing struct. Thank you, @glenjamin!