Add admin enforcement to protected branches API (#606) #610

amirilovic · 2017-04-01T05:07:45Z

Helps #606.

amirilovic · 2017-04-04T09:30:31Z

@shurcooL Is anything else needed here?

dmitshur · 2017-04-05T02:16:09Z

github/repos.go

@@ -511,13 +511,15 @@ type Branch struct {
 type Protection struct {
 	RequiredStatusChecks       *RequiredStatusChecks       `json:"required_status_checks"`
 	RequiredPullRequestReviews *RequiredPullRequestReviews `json:"required_pull_request_reviews"`
+	EnforceAdmins              *AdminEnforcement           `json:"enforce_admins"`
 	Restrictions               *BranchRestrictions         `json:"restrictions"`


The names of other structs here take pretty closely after the field name itself.

Have you considered naming this struct something like EnforceAdmins? If so, what made you pick AdminEnforcement instead?

It seems like the latter sounds better, but I'm a little unsure about the deviation from the pattern. Do you have any comments on this?

I'm on the fence about this, that's why I want to ask for your thoughts.

I choose AdminEnforcement because of: https://developer.github.com/v3/repos/branches/#get-admin-enforcement-of-protected-branch
The GET request states: Get admin enforcement of protected branch, which makes sense because it is actually a nested resource, not just a flag.

Sounds good, that's convincing rationale, thanks for providing it.

dmitshur · 2017-04-05T02:26:21Z

github/repos.go

+// AdminEnforcement represents the configuration to enforce required status checks for repository administrators.
+type AdminEnforcement struct {
+	URL     *string `json:"url,omitempty"`
+	Enabled bool    `json:"enabled"`


What's the reason URL is a pointer *string and has ,omitempty, unlike Enabled?

I think I get the reason, but it seems a bit arbitrary. I don't have any suggestions for improvement, so this is okay with me.

Hi @amirilovic, I'm trying to understand your approach here. If you won't mind, could you response to shurcooL question? I'm new in Go world I'm still trying to figure it out what is the best use-case for pointers fields and type fields in structs

@navarrano See #537 and the issues it mentions for background information on this.

The TLDR is that this is a style choice, both ways work. Using pointers allows users to know whether the value was actually present in the GitHub response or not, but they have to deal with a pointer. Using values means you can't know if it's a zero value or it was present in GH response, but no pointers to deal with.

My understanding is that Enabled is supposed to be a field that's always included in the response, so it's kinda okay not making it a pointer. But URL is expected to be more optional, and it might be worth knowing if it's included or not.

It also has to do with consistency with other similar structs for this API.

But it could've gone in another way.

@shurcooL @navarrano sorry for late response, @shurcooL that was exactly my reasoning, don't know if you had similar cases, but it seemed logical to me.

dmitshur · 2017-04-05T02:31:09Z

github/repos.go

@@ -538,6 +540,12 @@ type RequiredPullRequestReviews struct {
 	IncludeAdmins bool `json:"include_admins"`


In order to fully resolve #606, I think you should mark IncludeAdmins as deprecated, or remove it.

Same for IncludeAdmins inside RequiredStatusChecks struct.

If you deprecate it, then change PR description to "Helps #606.", so it won't be closed. We want to use that issue to track the entire change for https://developer.github.com/changes/2017-03-22-protected-branches-admin-improvements/, which includes removing the old way of setting admin.

@shurcooL I'm not sure about deprecation, seems like an overhead, maybe it's better just to remove old fields.

Feel free to go for it and make that a part of this PR.

@shurcooL struct RequiredPullRequestReviews should be removed completely?

As I understand, we want to remove IncludeAdmins fields. I see that RequiredPullRequestReviews contains only that field, and no others, so I understand where your question is coming from.

I would suspect that yes, we want to remove RequiredPullRequestReviews, since it has no other fields left, but the GitHub API docs say some strange/conflicting things right now:

GitHub recommends passing required_pull_request_reviews and enforce_admins. Passing these fields is not currently required, but may be required soon.

I'm not sure if that's out of date or a typo...

For next step, I'd recommend either settling by marking IncludeAdmins as deprecated for now (instead of removing them), and wait for the situation to be more clear. Or, better, email GitHub support and try to clarify the situation. They seek feedback regarding these preview API changes:

If you have any questions or feedback, please let us know!

I'll contact support and try out how API behaves currently if no value is set for RequiredPullRequestReviews

@shurcooL No reply from support yet, but here are test results:

When required_pull_request_reviews is not in the request, response status is 200 OK - existing required_pull_request_reviews is not affected

When required_pull_request_reviews is null, response is 200 OK - existing required_pull_request_reviews is removed from the resource

When required_pull_request_reviews is {}, response is 422 Unprocessable Entity, with following error in body:

{ "message": "Invalid request.\n\nNo subschema in \"anyOf\" matched.\n\"include_admins\" wasn't supplied.\nNot all subschemas of \"allOf\" matched.\nFor 'anyOf/1', {} is not a null.", "documentation_url": "https://developer.github.com/v3/repos/branches/#update-branch-protection" }

So, it's safe to remove required_pull_request_reviews from the request completely.

For required_status_checks:

When required_status_checks is sent with include_admins, response is 200 OK - existing required_status_checks is updated with new values

When required_status_checks is sent without include_admins, response is 422 Unprocessable Entity, with following error in body:

{ "message": "Invalid request.\n\nNo subschema in \"anyOf\" matched.\n\"include_admins\" wasn't supplied.\nNot all subschemas of \"allOf\" matched.\nFor 'anyOf/1', {\"url\"=>\"https://github.com/api/repos/amirilovic/test-repo/branches/master/protection/required_status_checks\", \"strict\"=>true, \"contexts\"=>[], \"contexts_url\"=>\"https://github.com/api/repos/amirilovic/test-repo/branches/master/protection/required_status_checks/contexts\"} is not a null.", "documentation_url": "https://developer.github.com/v3/repos/branches/#update-branch-protection" }

When required_status_checks is sent with include_admins = null, response is 422 Unprocessable Entity, with following error in body:

{ "message": "Invalid request.\n\nNo subschema in \"anyOf\" matched.\nFor 'properties/include_admins', nil is not a boolean.\nNot all subschemas of \"allOf\" matched.\nFor 'anyOf/1', {\"url\"=>\"https://github.com/api/repos/amirilovic/test-repo/branches/master/protection/required_status_checks\", \"strict\"=>true, \"include_admins\"=>nil, \"contexts\"=>[], \"contexts_url\"=>\"https://github.com/api/repos/amirilovic/test-repo/branches/master/protection/required_status_checks/contexts\"} is not a null.", "documentation_url": "https://developer.github.com/v3/repos/branches/#update-branch-protection" }

So, required_status_checks has to include include_admins field.

The RequiredStatusChecks.IncludeAdmins field is now documented both as required and deprecated.

It's pretty puzzling to me how to use the new API. There are two bools, do they need to be set to the same value if true, or does EnforceAdmins take higher precedence over RequiredStatusChecks.IncludeAdmins? Or can either one be set to true?

@shurcooL I've checked how it works. When setting either RequiredStatusChecks.IncludeAdmins,RequiredPullRequestReviews.IncludeAdmins or EnforceAdmins will set all other properties to the same value - changing one changes all the rest. RequiredPullRequestReviews.IncludeAdmins and RequiredStatusChecks.IncludeAdmins are required fields in terms of request structure correctness (request fails with 422 Unprocessable Entity, if they are not included - explained in previous comment), but logically they shouldn't be used since EnforceAdmins overrides them both. So, they are required, but deprecated by EnforceAdmins.

I still think this is confusing Go API for go-github users. But, this falls on GitHub, and it's temporary. According to https://developer.github.com/changes/2017-03-22-protected-branches-admin-improvements/, they'll remove the deprecated include_admins field on May 2, 2017, so this Go API will become less confusing then.

So, this is okay by me, I won't request further improvements (I can think of some, but I don't think it's worth the overhead).

dmitshur · 2017-04-05T02:33:30Z

Whoops, I self-requested a review as a way of not forgetting to deal with this PR, but I forgot anyway. Thanks for reminding.

…iredStatusChecks

dmitshur

I've spotted a minor documentation issue.

Other than that, do you think this PR is ready for final review and merging @amirilovic, or are we waiting on a reply from GitHub support before merging?

dmitshur · 2017-04-08T17:18:44Z

github/repos.go

 }

 // RequiredStatusChecks represents the protection status of a individual branch.
 type RequiredStatusChecks struct {
+	// Deprecated: Use EnforceAdmins instead.
 	// Enforce required status checks for repository administrators. (Required.)


You can't start the comment with "Deprecated:" first, because that breaks the Go style requirement of having documentation begin with identifier name. Put it second:

// Enforce required status checks for repository administrators. (Required.) // Deprecated: Use EnforceAdmins instead.

Reference: golang/go#10909 (comment).

amirilovic · 2017-04-09T18:08:31Z

@shurcooL I've fixed the comment issue. I think there is no need to wait for response from support, I've tested current behavior of the API. If something comes up from GH support, I'll report the issue and fix it.

dmitshur · 2017-04-10T20:54:57Z

Okay. I tried to review the PR and came up with a question at #610 (comment).

dmitshur · 2017-04-13T17:38:31Z

Also, see #617, which might be relevant here. /cc @gmlewis

It seems to add dismissal_restrictions to required_pull_request_reviews, so perhaps we don't want to remove required_pull_request_reviews anymore.

… in RequiredStatusChecks" This reverts commit 02a1f4d.

amirilovic · 2017-04-14T00:09:36Z

@shurcooL I've rolled back changes removing RequiredPullRequestReviews, also added deprecation comment for RequiredPullRequestReviews.IncludeAdmins. I've responded to your question about relationship between RequiredPullRequestReviews.IncludeAdmins, RequiredStatusChecks.IncludeAdmins and EnforceAdmins.

amirilovic · 2017-04-21T00:19:54Z

@shurcooL Anything else that needs to be done here?

dmitshur · 2017-04-21T00:21:06Z

Thanks for making progress on this. I'll try to review this soon.

dmitshur

I've edited the PR description to say "Helps #606", because we don't want merging this PR to close that issue just yet. It'll be fully resolved once include_admins can be removed.

LGTM. Thanks @amirilovic.

As I understand it, this API is somewhat confusing, but it's temporary, and it's a part of API preview, which means these things are normal and expected. That's why we can proceed with this and update it later.

gmlewis

LGTM. Thank you, @amirilovic and @shurcooL!

dmitshur · 2017-04-21T19:13:43Z

Thanks a lot @amirilovic!

Based on what it says at https://developer.github.com/changes/2017-03-22-protected-branches-admin-improvements/, we'll need to wait until May 2, 2017 to be able to completely resolve #606.

This change adds support for an addition to the Protected Branches API preview, the ability to turn on and off admin settings in one place. This is documented at https://developer.github.com/changes/2017-03-22-protected-branches-admin-improvements/. The IncludeAdmins fields are deprecated, but they're still required to be sent in the request for now. A future update to API preview will remove the IncludeAdmins fields completely. Helps google#606.

Add admin enforcement to protected branches API (google#606)

c00eead

dmitshur self-requested a review April 1, 2017 05:20

dmitshur reviewed Apr 5, 2017

View reviewed changes

Remove RequiredPullRequestReviews and deprecate IncludeAdmins in Requ…

02a1f4d

…iredStatusChecks

dmitshur requested changes Apr 8, 2017

View reviewed changes

dmitshur added the waiting for reply label Apr 8, 2017

Fix comment order

62a5b3a

dmitshur removed the waiting for reply label Apr 9, 2017

Revert "Remove RequiredPullRequestReviews and deprecate IncludeAdmins…

9237137

… in RequiredStatusChecks" This reverts commit 02a1f4d.

dmitshur approved these changes Apr 21, 2017

View reviewed changes

dmitshur requested a review from gmlewis April 21, 2017 00:53

gmlewis approved these changes Apr 21, 2017

View reviewed changes

dmitshur merged commit 2966f25 into google:master Apr 21, 2017

amirilovic deleted the protected-branches-admin-enforcement-606 branch April 24, 2017 04:17

gmlewis mentioned this pull request Sep 5, 2017

Fix #664. Docs for empty DismissalRestrictions were incorrect. [] -> {} #703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add admin enforcement to protected branches API (#606) #610

Add admin enforcement to protected branches API (#606) #610

amirilovic commented Apr 1, 2017 •

edited by dmitshur

Loading

amirilovic commented Apr 4, 2017

dmitshur Apr 5, 2017

amirilovic Apr 5, 2017

dmitshur Apr 5, 2017

dmitshur Apr 5, 2017

jkosecki Apr 6, 2017 •

edited

Loading

dmitshur Apr 6, 2017

amirilovic Apr 6, 2017

dmitshur Apr 5, 2017 •

edited

Loading

amirilovic Apr 5, 2017

dmitshur Apr 5, 2017

amirilovic Apr 6, 2017

dmitshur Apr 6, 2017 •

edited

Loading

amirilovic Apr 6, 2017

amirilovic Apr 6, 2017 •

edited

Loading

dmitshur Apr 10, 2017

amirilovic Apr 14, 2017 •

edited

Loading

dmitshur Apr 21, 2017

dmitshur commented Apr 5, 2017

dmitshur left a comment •

edited

Loading

dmitshur Apr 8, 2017

amirilovic commented Apr 9, 2017

dmitshur commented Apr 10, 2017

dmitshur commented Apr 13, 2017 •

edited

Loading

amirilovic commented Apr 14, 2017

amirilovic commented Apr 21, 2017

dmitshur commented Apr 21, 2017

dmitshur left a comment

gmlewis left a comment

dmitshur commented Apr 21, 2017

		@@ -538,6 +540,12 @@ type RequiredPullRequestReviews struct {
		IncludeAdmins bool `json:"include_admins"`

Add admin enforcement to protected branches API (#606) #610

Add admin enforcement to protected branches API (#606) #610

Conversation

amirilovic commented Apr 1, 2017 • edited by dmitshur Loading

amirilovic commented Apr 4, 2017

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

jkosecki Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Apr 5, 2017 • edited Loading

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

dmitshur Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirilovic Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirilovic Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Apr 5, 2017

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirilovic commented Apr 9, 2017

dmitshur commented Apr 10, 2017

dmitshur commented Apr 13, 2017 • edited Loading

amirilovic commented Apr 14, 2017

amirilovic commented Apr 21, 2017

dmitshur commented Apr 21, 2017

dmitshur left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

dmitshur commented Apr 21, 2017

amirilovic commented Apr 1, 2017 •

edited by dmitshur

Loading

jkosecki Apr 6, 2017 •

edited

Loading

dmitshur Apr 5, 2017 •

edited

Loading

dmitshur Apr 6, 2017 •

edited

Loading

amirilovic Apr 6, 2017 •

edited

Loading

amirilovic Apr 14, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur commented Apr 13, 2017 •

edited

Loading