Adds required pull request reviews by alindeman · Pull Request #512 · google/go-github

alindeman · 2017-01-09T02:32:59Z

required_pull_request_reviews is a recently introduced object that will soon be required:

/cc: @sr

`required_pull_request_reviews` is a newly introduced object: * https://developer.github.com/v3/repos/branches/#get-branch-protection * https://developer.github.com/v3/repos/branches/#update-branch-protection

dmitshur

I have one question about omitempty. Everything else looks good.

dmitshur · 2017-01-09T03:28:00Z

github/repos.go

+// RequiredPullRequestReviews represents the protection configuration for pull requests.
+type RequiredPullRequestReviews struct {
+	// Enforce pull request reviews for repository administrators.
+	IncludeAdmins *bool `json:"include_admins,omitempty"`


include_admins key is documented as:

Required. Enforce required status checks for repository administrators.

Shouldn't we not use omitempty option for required parameters?

Thanks for bringing this up.

I was mostly following what I thought was a convention. As another example, something like creating a repository requires a name in the same manner, but name is omitempty on the Repository struct. It seems this pattern is pretty common in this codebase.

I also initially thought that in the absence of omitempty, failing to provide a value would send null along which might evaluate to false server-side, and therefore be surprising. However, on actually testing this assumption, it appears that GitHub validates that include_admins is strictly a boolean and will raise an error if null is passed. The assumption was wrong.

If you think that going forward it's a good idea for required values to, well, omit omitempty, I'm on board and will make it so for this property. Let me know what you think.

Yeah, I saw that similar fields above that are required also have omitempty, and I figured you probably just followed the pattern. I wanted to bring it up so we could discuss the best thing to do from now on.

I do think leaving out omitempty would be a more appropriate thing to do, and it's better to start doing it late than never. There are at least a few places where it's already done, e.g., see here:

go-github/github/projects.go

Lines 169 to 170 in dcda0b9

// The name of the project column. (Required for creation and update.)

Name string `json:"name"`

go-github/github/pulls.go

Line 291 in dcda0b9

CommitMessage string `json:"commit_message"`

What if we go a step further to increase the mapping between a required field and it always being included by making it a value rather than pointer? E.g.:

IncludeAdmins bool `json:"include_admins"` // Required.

That eliminates any invalid value such as "null" being possible.

Thoughts?

@shurcooL 👍 I like that it exposes the requirements of the API in the type itself. I've made it so.

I also pushed 2a12ccd to backport this convention to RequiredStatusChecks because it also includes similarly required parameters like include_admins. I realize this is a breaking change, but I feel like it's a good idea to remain consistent within the structures that the {Create,Update}BranchProtection functions use. Furthermore, the GitHub HTTP API is itself still beta and open to breaking changes. If you think it's a bad idea, though, I can lop that commit off.

I like that it exposes the requirements of the API in the type itself. I've made it so.

Great!

If you think it's a bad idea, though, I can lop that commit off.

Not at all, on the contrary, I think that's absolutely the way to go. Thank you for doing that.

Yes, the GitHub API is still in beta so I think it's fine to break it slightly, since it's an improvement.

/cc @varadarajana FYI, see above. You'll need to make a small change to your PR #509 as a result.

By making it a value, rather than a pointer to a value, we drive home the point that it must be specified and is always present in responses.

Required fields should be values, not pointers, because a lack of a value is an invalid specification.

dmitshur · 2017-01-11T04:27:38Z

Hmm, what about BranchRestrictionsRequest?

I can see that in https://developer.github.com/v3/repos/branches/#update-branch-protection it has 2 keys, and unlike the required_status_checks and required_pull_request_reviews, they don't have Required. text. However, it does say the following for all 3:

The required_status_checks object must have the following keys

The required_pull_request_reviews object must have the following keys

The restrictions object must have the following keys

Emphasis mine. It kinda sounds like it's implied that they're also required... Not sure what to do here. Do you think it's worth clarifying with GitHub support, @alindeman?

dmitshur · 2017-01-11T04:36:28Z

Another observation I want to point out. Both Protection and ProtectionRequest reuse the same structs for RequiredStatusChecks and RequiredPullRequestReviews.

It's true that the fields in those structs are documented as required for the https://developer.github.com/v3/repos/branches/#update-branch-protection endpoint, but the change from pointer to value will affect the Protection object used during unmarshaling. We are assuming that since the input fields are required, they should be always present as part of the output from https://developer.github.com/v3/repos/branches/#get-branch-protection endpoint too.

If that isn't the case, it would no longer be possible to detect if a zero value came from the response, or if the response didn't contain a value.

This is quite subtle, and I think it's not going to be a problem, but I just wanted to point it out for review purposes.

dmitshur

See 2 comments above. We might want to also change BranchRestrictionsRequest struct.

Aside from that, this change LGTM as it currently stands.

gmlewis · 2017-01-11T05:39:40Z

Thank you, @alindeman and @shurcooL!
It seems to me like your interpretation of the GitHub API docs is totally reasonable and that we can run with it. We can always revisit if we discover issues.
LGTM.
I'll let this sit for a day if there are any other comments, then merge.

alindeman · 2017-01-12T13:00:45Z

@shurcooL I didn't get to look into your questions yesterday like I hoped, but I'll give these a look later today my time (US East)

alindeman · 2017-01-13T03:05:39Z

I can see that in https://developer.github.com/v3/repos/branches/#update-branch-protection it has 2 keys, and unlike the required_status_checks and required_pull_request_reviews, they don't have Required. text. However, it does say the following for all 3:

The required_status_checks object must have the following keys
The required_pull_request_reviews object must have the following keys
The restrictions object must have the following keys
Emphasis mine. It kinda sounds like it's implied that they're also required... Not sure what to do here. Do you think it's worth clarifying with GitHub support, @alindeman?

I explored the API by sending it some different permutations of requests and found that, you're right, users and teams are required if restrictions is passed a non-null value. And if it's passed, the response always contains the users and teams as well. I'll make them non-pointers as well in an upcoming commit.

alindeman · 2017-01-13T03:11:39Z

If that isn't the case, it would no longer be possible to detect if a zero value came from the response, or if the response didn't contain a value.

This is quite subtle, and I think it's not going to be a problem, but I just wanted to point it out for review purposes.

Agreed. While I think it's true that it would be possible for the GitHub API to return only a subset of the required keys in responses, I think in practice it would be a bug given the constraints they've documented and appear to be enforcing server-side with JSON Schema.

gmlewis · 2017-01-13T17:48:21Z

LGTM.
Merging.

gmlewis · 2017-01-13T17:52:32Z

Integrated in 5e7fada

All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection. In #512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required. Additionally, note that empty slices must be non-nil. From running integration tests, I've seen that passing nil slices (which get encoded as null in via encoding/json) resulted in invalid request errors such as: No subschema in "anyOf" matched. For 'properties/users', nil is not an array. For 'properties/teams', nil is not an array. Not all subschemas of "allOf" matched. For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. [] Follows #512.

All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection. In #512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required. Additionally, note that empty slices must be non-nil. From running integration tests, I've seen that passing nil slices (which get encoded as null via encoding/json) resulted in invalid request errors such as: No subschema in "anyOf" matched. For 'properties/users', nil is not an array. For 'properties/teams', nil is not an array. Not all subschemas of "allOf" matched. For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. [] Follows #512.

`required_pull_request_reviews` is a newly introduced object: * https://developer.github.com/v3/repos/branches/#get-branch-protection * https://developer.github.com/v3/repos/branches/#update-branch-protection Closes google#512. Change-Id: Ia4d3ff3e39163fe58a691a8abf0a067bf3c714e5

All of those fields are documented as required at https://developer.github.com/v3/repos/branches/#update-branch-protection. In google#512, we changed the API to better reflect that. However, it should still be documented that the fields are required. This is consistent with the other required fields that are also documented as required. Additionally, note that empty slices must be non-nil. From running integration tests, I've seen that passing nil slices (which get encoded as null via encoding/json) resulted in invalid request errors such as: No subschema in "anyOf" matched. For 'properties/users', nil is not an array. For 'properties/teams', nil is not an array. Not all subschemas of "allOf" matched. For 'anyOf/1', {"users"=>nil, "teams"=>nil} is not a null. [] Follows google#512.

Adds required pull request reviews

1474de2

`required_pull_request_reviews` is a newly introduced object: * https://developer.github.com/v3/repos/branches/#get-branch-protection * https://developer.github.com/v3/repos/branches/#update-branch-protection

dmitshur reviewed Jan 9, 2017

View reviewed changes

alindeman added 2 commits January 10, 2017 20:39

include_admins is a required value

939e936

By making it a value, rather than a pointer to a value, we drive home the point that it must be specified and is always present in responses.

Aligns other structs with the new convention

2a12ccd

Required fields should be values, not pointers, because a lack of a value is an invalid specification.

alindeman force-pushed the required-pull-request-reviews branch from 7634edb to 2a12ccd Compare January 11, 2017 01:47

dmitshur approved these changes Jan 11, 2017

View reviewed changes

gmlewis added the lgtm label Jan 11, 2017

users and teams are required keys

6252e7a

dmitshur approved these changes Jan 13, 2017

View reviewed changes

gmlewis closed this in 5e7fada Jan 13, 2017

dmitshur mentioned this pull request Jan 17, 2017

Document required branch protection fields. #519

Merged

alindeman mentioned this pull request Jan 30, 2017

Adds support for GET /repos/:owner/:repo/collaborators/:username/permission #534

Closed

dmitshur mentioned this pull request Jan 31, 2017

Policy on using pointers/values in structs for receiving data from GitHub servers. #537

Closed

	// The name of the project column. (Required for creation and update.)
	Name string `json:"name"`

Conversation

alindeman commented Jan 9, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

alindeman Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alindeman Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Jan 11, 2017

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Jan 11, 2017

Uh oh!

dmitshur commented Jan 11, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Jan 11, 2017

Uh oh!

alindeman commented Jan 12, 2017

Uh oh!

alindeman commented Jan 13, 2017

Uh oh!

alindeman commented Jan 13, 2017

Uh oh!

gmlewis commented Jan 13, 2017

Uh oh!

gmlewis commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmitshur Jan 9, 2017 •

edited

Loading

alindeman Jan 11, 2017 •

edited

Loading