Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 21, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c00eead
Add admin enforcement to protected branches API (#606)
amirilovic Apr 1, 2017
02a1f4d
Remove RequiredPullRequestReviews and deprecate IncludeAdmins in Requ…
amirilovic Apr 6, 2017
62a5b3a
Fix comment order
amirilovic Apr 9, 2017
9237137
Revert "Remove RequiredPullRequestReviews and deprecate IncludeAdmins…
amirilovic Apr 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions github/github-accessors.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions github/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,19 +511,22 @@ 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"`
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}

// ProtectionRequest represents a request to create/edit a branch's protection.
type ProtectionRequest struct {
RequiredStatusChecks *RequiredStatusChecks `json:"required_status_checks"`
RequiredPullRequestReviews *RequiredPullRequestReviews `json:"required_pull_request_reviews"`
EnforceAdmins bool `json:"enforce_admins"`
Restrictions *BranchRestrictionsRequest `json:"restrictions"`
}

// RequiredStatusChecks represents the protection status of a individual branch.
type RequiredStatusChecks struct {
// Enforce required status checks for repository administrators. (Required.)
// Deprecated: Use EnforceAdmins instead.
IncludeAdmins bool `json:"include_admins"`
// Require branches to be up to date before merging. (Required.)
Strict bool `json:"strict"`
Expand All @@ -535,9 +538,16 @@ type RequiredStatusChecks struct {
// RequiredPullRequestReviews represents the protection configuration for pull requests.
type RequiredPullRequestReviews struct {
// Enforce pull request reviews for repository administrators. (Required.)
// Deprecated: Use EnforceAdmins instead.
IncludeAdmins bool `json:"include_admins"`
Copy link
Member

@dmitshur dmitshur Apr 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shurcooL struct RequiredPullRequestReviews should be removed completely?

Copy link
Member

@dmitshur dmitshur Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@amirilovic amirilovic Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@amirilovic amirilovic Apr 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jkosecki jkosecki Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

}

// BranchRestrictions represents the restriction that only certain users or
// teams may push to a branch.
type BranchRestrictions struct {
Expand Down
6 changes: 5 additions & 1 deletion github/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {

testMethod(t, r, "GET")
testHeader(t, r, "Accept", mediaTypeProtectedBranchesPreview)
fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
fmt.Fprintf(w, `{"required_status_checks":{"include_admins":true,"strict":true,"contexts":["continuous-integration"]},"required_pull_request_reviews":{"include_admins":true},"enforce_admins":{"url":"/repos/o/r/branches/b/protection/enforce_admins","enabled":true},"restrictions":{"users":[{"id":1,"login":"u"}],"teams":[{"id":2,"slug":"t"}]}}`)
})

protection, _, err := client.Repositories.GetBranchProtection(context.Background(), "o", "r", "b")
Expand All @@ -499,6 +499,10 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) {
RequiredPullRequestReviews: &RequiredPullRequestReviews{
IncludeAdmins: true,
},
EnforceAdmins: &AdminEnforcement{
URL: String("/repos/o/r/branches/b/protection/enforce_admins"),
Enabled: true,
},
Restrictions: &BranchRestrictions{
Users: []*User{
{Login: String("u"), ID: Int(1)},
Expand Down