Update Branch Protection to current GitHub API #790

s-t-e-v-e-n-k · 2018-05-14T06:51:22Z

The Branch protection API has been radically changed by GitHub, add new
methods to Branch reflecting it. Branch protection methods are now
called on the Branch object itself, rather than awkwardly hanging off the
Repository object and requiring branch names to be passed in.

This adds an over-arching methods to get, edit and remove protection
entirely, as well as fine-grained methods for getting, setting and
dropping certain aspects of branch protection, as well as three new
objects to encompass querying those aspects.

This also destroys Repository.protect_branch(), the endpoint has been
removed. The old-style protection attributes on Branch have also been
removed to force users onto the new API, since they are still sent, but
no longer populated.

Fixes #586

--

I'm sorry for just how large this branch is. I tried to split it up, but everything depends on the new methods in Branch, which pretty much pulls in everything ever due to the tests.

JPWKU · 2018-05-14T20:58:25Z

@s-t-e-v-e-n-k I am reviewing, but will take a few hours at least.

JPWKU

Chunking my review, I still have more to do. In general for cleanup can we wrap lines to no more than 72 chars.

JPWKU · 2018-05-14T21:26:03Z

github/Branch.py

+        else:
+            post_parameters["enforce_admins"] = None
+
+        if dismissal_users is not github.GithubObject.NotSet or dismissal_teams is not github.GithubObject.NotSet or dismiss_stale_reviews is not github.GithubObject.NotSet or require_code_owner_reviews is not github.GithubObject.NotSet:


Wrap at 72 characters for readablility.

required_approving_review_count is a parameter available here for required_pull_request_reviews object as well.

"The Protected Branches API now has a setting for requiring a specified number of approving pull request reviews before merging. This feature is currently available for developers to preview."

This requires sending a custom Accept header which the code doesn't do. I do plan to add it, but this branch was already large enough.

JPWKU · 2018-05-14T21:27:53Z

github/Branch.py

+        )
+        return github.BranchProtection.BranchProtection(self._requester, headers, data, completed=True)
+
+    def edit_protection(self, strict=github.GithubObject.NotSet, contexts=github.GithubObject.NotSet, enforce_admins=github.GithubObject.NotSet, dismissal_users=github.GithubObject.NotSet, dismissal_teams=github.GithubObject.NotSet, dismiss_stale_reviews=github.GithubObject.NotSet, require_code_owner_reviews=github.GithubObject.NotSet, user_push_restrictions=github.GithubObject.NotSet, team_push_restrictions=github.GithubObject.NotSet):


Line wrap for readability.

JPWKU · 2018-05-14T21:33:21Z

github/Branch.py

+                post_parameters["required_pull_request_reviews"]["dismissal_restrictions"]["teams"] = dismissal_teams
+        else:
+            post_parameters["required_pull_request_reviews"] = None
+        if user_push_restrictions is not github.GithubObject.NotSet or team_push_restrictions is not github.GithubObject.NotSet:


Perhaps we can give meaningful feedback if a user tries to set team push restrictions on a repo that is not part of an org?

To my mind, the exception in itself is useful enough here -- that code is checked in the tests.

JPWKU · 2018-05-14T21:34:03Z

github/Branch.py

+            input=post_parameters
+        )
+
+    def remove_protection(self):


JPWKU · 2018-05-14T21:35:42Z

github/Branch.py

+            input=post_parameters
+        )
+
+    def remove_required_status_checks(self):


JPWKU · 2018-05-14T21:45:58Z

github/Branch.py

+        )
+        return data["enabled"]
+
+    def set_admin_enforcement(self):


JPWKU · 2018-05-14T21:46:06Z

github/Branch.py

+            self.protection_url + "/enforce_admins"
+        )
+
+    def remove_admin_enforcement(self):


JPWKU · 2018-05-14T22:12:48Z

github/Branch.py

+            self.protection_url + "/enforce_admins"
+        )
+
+    def get_user_push_restrictions(self):


JPWKU · 2018-05-14T22:12:59Z

github/Branch.py

+            None
+        )
+
+    def get_team_push_restrictions(self):


JPWKU · 2018-05-14T22:14:13Z

github/Branch.py

+            input=users
+        )
+
+    def edit_team_push_restrictions(self, *teams):


JPWKU · 2018-05-31T13:57:03Z

Circling back to this today..been a busy week.

sfdye · 2018-06-04T02:29:25Z

Finally someone taking this up. Good job @s-t-e-v-e-n-k

stale · 2018-08-03T03:17:39Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jiang-wei · 2018-08-14T21:09:48Z

waiting for the merge for days ...

sfdye · 2018-08-15T01:42:14Z

@JPWKU I really want to get this out. Do you have anything to add for the code review?

JPWKU · 2018-08-15T02:36:12Z

@jiang-wei @sfdye I realize this has been in limbo for awhile. Just a combination of it being a large PR and having limited bandwidth to go over. Thanks for bringing it to the top again. @s-t-e-v-e-n-k Looks like there is a conflict, can you rebase and resolve that to see a test run again?

The Branch protection API has been radically changed by GitHub, add new methods to Branch reflecting it. Branch protection methods are now called on the Branch object itself, rather than awkwardly hanging off the Repository object and requiring branch names to be passed in. This adds an over-arching methods to get, edit and remove protection entirely, as well as fine-grained methods for getting, setting and dropping certain aspects of branch protection, as well as three new objects to encompass querying those aspects. This also destroys Repository.protect_branch(), the endpoint has been removed. The old-style protection attributes on Branch have also been removed to force users onto the new API, since they are still sent, but no longer populated. Fixes PyGithub#586

s-t-e-v-e-n-k · 2018-08-15T03:15:54Z

@JPWKU Done! Let's get this merged! \o/

sfdye · 2018-08-15T03:19:09Z

@JPWKU Any more comments?

JPWKU · 2018-08-15T03:23:50Z

Doing one last look through.

JPWKU

LGTM

sfdye · 2018-08-15T03:37:00Z

LGTM. @s-t-e-v-e-n-k Could you please add a summary for this PR, for the release note? I don't wanna miss anything 😄

s-t-e-v-e-n-k · 2018-08-15T04:02:06Z

@sfdye I think the following:

"Add support for the Branch Protection API, which adds 12 new methods onto github.Branch allowing repository owners to set protection on any branch, such as user or team push restrictions, or requiring a number of reviewers before a PR can be merged."

Is that enough, or do I need to go into more detail?

sfdye · 2018-08-15T04:03:51Z

It's fine, I will reformat to release note style bullet points.

sfdye · 2018-08-20T08:06:11Z

@s-t-e-v-e-n-k

I guess with this PR merged, Repository.get_protected_branch should also be removed right?

https://github.com/PyGithub/PyGithub/blob/080d807552718df5179e7998cc245e3bb9ea0262/github/Repository.py#L1314:L1325

s-t-e-v-e-n-k · 2018-08-20T09:16:04Z

@sfdye I had a PR that was marked as stale that deprecated it, but this PR already adds a breaking change, so I'll push up a PR tonight that removes it.

sfdye · 2018-08-20T09:17:25Z

Oh yeah, we can just remove it. I have issued a warning on the release note as well.

s-t-e-v-e-n-k force-pushed the branch-protection-update branch from 62ac94d to 4f01121 Compare May 14, 2018 12:22

JPWKU self-requested a review May 14, 2018 20:59

JPWKU reviewed May 14, 2018

View reviewed changes

stale bot added the stale label Aug 3, 2018

stale bot closed this Aug 10, 2018

sfdye reopened this Aug 10, 2018

stale bot removed the stale label Aug 10, 2018

s-t-e-v-e-n-k force-pushed the branch-protection-update branch from 4f01121 to 171cc56 Compare August 15, 2018 02:46

JPWKU approved these changes Aug 15, 2018

View reviewed changes

JPWKU merged commit a7c16c6 into PyGithub:master Aug 15, 2018

sfdye added the improvement label Aug 15, 2018

sfdye mentioned this pull request Aug 17, 2018

Branch object is missing most attributes and methods #867

Closed

s-t-e-v-e-n-k deleted the branch-protection-update branch October 21, 2021 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Branch Protection to current GitHub API #790

Update Branch Protection to current GitHub API #790

s-t-e-v-e-n-k commented May 14, 2018

JPWKU commented May 14, 2018

JPWKU left a comment

JPWKU May 14, 2018

JPWKU May 14, 2018

s-t-e-v-e-n-k May 15, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

s-t-e-v-e-n-k May 15, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU May 14, 2018

JPWKU commented May 31, 2018

sfdye commented Jun 4, 2018

stale bot commented Aug 3, 2018

jiang-wei commented Aug 14, 2018

sfdye commented Aug 15, 2018

JPWKU commented Aug 15, 2018

s-t-e-v-e-n-k commented Aug 15, 2018

sfdye commented Aug 15, 2018

JPWKU commented Aug 15, 2018

JPWKU left a comment

sfdye commented Aug 15, 2018

s-t-e-v-e-n-k commented Aug 15, 2018

sfdye commented Aug 15, 2018

sfdye commented Aug 20, 2018

s-t-e-v-e-n-k commented Aug 20, 2018

sfdye commented Aug 20, 2018

Update Branch Protection to current GitHub API #790

Update Branch Protection to current GitHub API #790

Conversation

s-t-e-v-e-n-k commented May 14, 2018

JPWKU commented May 14, 2018

JPWKU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPWKU commented May 31, 2018

sfdye commented Jun 4, 2018

stale bot commented Aug 3, 2018

jiang-wei commented Aug 14, 2018

sfdye commented Aug 15, 2018

JPWKU commented Aug 15, 2018

s-t-e-v-e-n-k commented Aug 15, 2018

sfdye commented Aug 15, 2018

JPWKU commented Aug 15, 2018

JPWKU left a comment

Choose a reason for hiding this comment

sfdye commented Aug 15, 2018

s-t-e-v-e-n-k commented Aug 15, 2018

sfdye commented Aug 15, 2018

sfdye commented Aug 20, 2018

s-t-e-v-e-n-k commented Aug 20, 2018

sfdye commented Aug 20, 2018