ProtectedBranches API changes for Required Review Enforcement

[ryangribble]

As per this blog anouncement the ProtectedBranches API has a new required_pull_request_reviews object

• On the existing UpdateBranchProtection method (add to our BranchProtectionSettingsUpdate request object, which will require a few ctors to be added to cater for the different combinations of the 3 fields being provided/not provided)

• Returned by the existing GetBranchProtection method (add to our BranchProtectionSettings response object)

• On new methods to Get, Update, Remove the required review enforcement

  • GET /repos/:owner/:repo/branches/:branch/protection/required_pull_request_reviews
  • PATCH /repos/:owner/:repo/branches/:branch/protection/required_pull_request_reviews
  • DELETE /repos/:owner/:repo/branches/:branch/protection/required_pull_request_reviews

[M-Zuber]

I'd like to try and do this one if it's still available

[ryangribble]

All yours @M-Zuber !

[ryangribble]

Unless you prefer the organization ones I also just raised #1513 and #1514 in which case Im happy to knock over this protected branches one since Im pretty familiar with this area.

Up to you though, if you want to do this one that's cool

[M-Zuber]

I want something small in order to get back into things.
Lets see if I can get this one done fast enough to start tackling those also 😀

[BigAlInTheHouse]

existen datos ahora. ALG Services

…

On Dec 12, 2016 7:52 AM, "Ryan Gribble" ***@***.***> wrote: As per this blog anouncement <https://developer.github.com/v3/repos/branches/#update-branch-protection> the ProtectedBranches API has a new required_pull_request_reviews object - On the existing UpdateBranchProtection method (add to our BranchProtectionSettingsUpdate request object, which will require a few ctors to be added to cater for the different combinations of the 3 fields being provided/not provided) - Returned by the existing GetBranchProtection method (add to our BranchProtectionSettings response object) - On new methods to Get <https://developer.github.com/v3/repos/branches/#get-pull-request-review-enforcement-of-protected-branch>, Update <https://developer.github.com/v3/repos/branches/#update-pull-request-review-enforcement-of-protected-branch>, Remove <https://developer.github.com/v3/repos/branches/#remove-pull-request-review-enforcement-of-protected-branch> the required review enforcement - GET /repos/:owner/:repo/branches/:branch/protection/required_ pull_request_reviews - PATCH /repos/:owner/:repo/branches/:branch/protection/required_ pull_request_reviews - DELETE /repos/:owner/:repo/branches/:branch/protection/required_ pull_request_reviews — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1512>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHT9JR2bogRk_V-YxTRgGfH2WI5lhKClks5rHVGzgaJpZM4LKkUR> .

[M-Zuber]

How exactly do you want the additional ctors to look?
Currently there are 2: one that takes only a BranchProtectionRequiredStatusChecksUpdate, and the other that takes a BranchProtectionPushRestrictionsUpdate in addition.

Based on my understanding of the docs: You can, and should, pass required_pull_request_reviews as well. While this is not required currently, it should be treated as such and will be soon., the BranchProtectionRequiredPullRequestReviewsUpdate object is different in that it can not be disabled by passing in null.

This would lead to a total of 3 ctors:

• BranchProtectionRequiredPullRequestReviewsUpdate + BranchProtectionRequiredStatusChecksUpdate
• BranchProtectionRequiredPullRequestReviewsUpdate + BranchProtectionPushRestrictionsUpdate
• All three

Does that make sense?

[ryangribble]

I would have thought passing null means review isn't required at all, then passing the object means review IS required (with the boolean indicating whether admins also have to comply with it or not). If you can't pass null how do you indicate no review is required?

[M-Zuber]

Hmmm... Had not thought of it that way, and you are probably correct.
But the docs seem to imply otherwise as for the older params they specify pass in null to disable while for the new param they don't.

So I am going to assume you are correct, and try to write an integration test proving it.

[M-Zuber]

I didn't get to writing the tests yet, but am making a pr just to get another set of 👀 on it as soon as possible

[nickfloyd]

resolved with: #1523