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

beiriannydd · 2017-08-20T19:28:06Z

I've driveby added the require owner commit attribute as an omitmissing as it
was missing from the API. Because it is omitmissing it is not a breaking change.

Added spew library which helped me discover that we were comparing returned URLs from the
AdminEnforcement object. None of the other objects in the API care about their URLs so
I just removed that.

beiriannydd · 2017-08-20T19:31:21Z

So, tests fail, looks as though that's because I added spew. It really helped me debug, if you want me to remove then I can do that.

beiriannydd · 2017-08-21T01:28:48Z

github/repos.go

-
-// MarshalJSON implements the json.Marshaler interface.
-// Converts nil value of PullRequestReviewsEnforcementRequest.DismissalRestrictionsRequest to empty array
-func (req PullRequestReviewsEnforcementRequest) MarshalJSON() ([]byte, error) {


No longer necessary as we don't need to hack an empty object into an empty array.

Can you explain why it's no longer needed to be done?

At https://developer.github.com/v3/repos/branches/#update-branch-protection, it still says:

You can disable dismissal restrictions by passing the dismissal_restrictions object as an empty array.

Which is why we did this in the first place in #637.

Ivan Žužak [email protected] | Ivan Žužak [email protected] | Aug 10 (12 days ago)
Thanks so much for reporting this, Faye -- I believe that's a typo in the docs. Instead of >passing an empty array [], you should pass in an empty object {}. Can you give that a try >and let us know if it help?

Yep, that is the issue alright. I tested your suggestion and it worked. Thanks!

Perhaps I should have made a PR against their API docs before this one? ;)

Can you please provide more information about this? It looks like you've pasted a portion of a reply from github support. Can you post more context?

I think I understand what's happening, but I want to confirm. My understanding is that github support has confirmed on August 10th that the https://developer.github.com/v3/repos/branches/#update-branch-protection GitHub documentation has a typo in:

You can disable dismissal restrictions by passing the dismissal_restrictions object as an empty array.

And they meant to write:

You can disable dismissal restrictions by passing the dismissal_restrictions object as an empty object.

Is that what's happening here?

Yes that is exactly right.

Did GitHub support say if they're planning to update/fix the documentation on their side? It's been 12 days and https://developer.github.com/v3/repos/branches/#update-branch-protection still says empty array rather than empty object.

beiriannydd · 2017-08-21T01:30:43Z

github/repos.go

@@ -609,8 +589,7 @@ type PullRequestReviewsEnforcementUpdate struct {

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


This is the only time I've seen the returned URL from github be used in a structure in this code. It is hard to test too. It was causing my test to fail while correct because URL was being compared as nil.

beiriannydd · 2017-08-21T01:31:39Z

github/repos_test.go

@@ -917,20 +914,6 @@ func TestRepositoriesService_RemoveAdminEnforcement(t *testing.T) {
 	}
 }

-func TestPullRequestReviewsEnforcementRequest_MarshalJSON_nilDismissalRestirctions(t *testing.T) {


No need to test this function which we've removed.

dmitshur · 2017-08-22T15:26:39Z

test/integration/repos_test.go

-		// Using spew helped me see that we were comparing an unwanted URL as part of
-		// AdminEnforcement structure.  Maybe add to other %+v tests?
-		t.Error(spew.Sprintf("Repositories.UpdateBranchProtection() returned %+v, want %+v", protection, want))
+		t.Errorf("Repositories.UpdateBranchProtection() returned %+v, want %+v", protection, want)


We have something similar to spew already available, you can/should use it instead.

See https://godoc.org/github.com/google/go-github/github#Stringify.

Cool I will look at that.

dmitshur · 2017-08-22T15:35:42Z

Thanks for working on this PR @beiriannydd.

Before proceeding, we should look at the reason [] was being used in the first place and confirm that the same reason is no longer valid.

I was hasty in applying the bug label to issue #664, that's my fault. I think we first need to confirm this is a bug. As you wrote in a code comment, it might be that we're getting 422 error because of an invalid precondition, like trying to use team/user permissions on a non-organization repo.

Basically, how we confirm that the previous behavior is not valid and the new one is valid? How did you test this PR?

beiriannydd · 2017-08-22T20:29:53Z

I needed to make a change to make my code which operates on our organization github repos and protects branches work. I originally fixed it by replacing []interface{}{} with map[string]interface{}{} but then it dawned on me that this was all to work around the normal behavior. The updateBranch test was failing integration and after the patch it doesn't fail which means it is happy with what we sent, and we get back what we expect in this situation.

beiriannydd · 2017-08-22T20:53:53Z

github/repos.go

 }

 // PullRequestReviewsEnforcementRequest represents request to set the pull request review
 // enforcement of a protected branch. It is separate from PullRequestReviewsEnforcement above
 // because the request structure is different from the response structure.
 type PullRequestReviewsEnforcementRequest struct {
 	// Specifies which users and teams should be allowed to dismiss pull requets reviews. Can be nil to disable the restrictions.
-	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"`
+	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions,omitempty"`


Your point has made me question if I am being too liberal with omitempty here.

Yes, it looks out of place. The comment says:

Can be nil to disable the restrictions.

And I suspect it needs to be without omitempty to function. That way, using a nil value for DismissalRestrictionsRequest field has a chance of disabling the restrictions.

However, I can't be certain until I understand the change at large.

My question is whether there is a tristate behavior:
1 present and {} = disable existing setting (cannot be tested on non-org repo)
2 not present = leave existing setting? (hard to test on non-org repo) but we know that it is not an error.
3 present and {users:[], groups:[]} = configuring new settings.
I have tested 1 & 3 but I don't want to use my company's org account for testing 2 ;)

beiriannydd · 2017-08-22T20:55:19Z

test/integration/repos_test.go

-				Teams: []string{},
-			},
-			DismissStaleReviews: true,
+			// Nil is perfectly acceptable here now and it proves the test.


Although I guess the question is whether when the branch was protected with users in this list and we want to remove them, if that will happen with this configuration or if it would be a noop.

beiriannydd · 2017-08-24T17:54:47Z

github/repos.go

-	// Specifies which users and teams should be allowed to dismiss pull requets reviews. Can be nil to disable the restrictions.
-	DismissalRestrictionsRequest *DismissalRestrictionsRequest `json:"dismissal_restrictions"`
+	// Specifies which users and teams should be allowed to dismiss pull requets reviews.
+	// MUST be nil for non-organization repositories, may be empty on an organization repo


This clarification is based on additional work I did to make it possible to pass an empty DismissalRestrictionsRequest{} and have that convert to a json empty object. Returning it to that state causes it to 422 again (as expected), but it works when used on an org repo (internal application testing)

beiriannydd · 2017-08-26T17:40:29Z

Hopefully someone can add the $COVERALLS_TOKEN to the build box soon.

willnorris · 2017-08-27T02:49:54Z

Hopefully someone can add the $COVERALLS_TOKEN to the build box soon.

Strange, it was working before. I'll drop the coveralls code for now so it doesn't break Travis test reporting.

willnorris · 2017-08-27T02:54:43Z

try rebasing onto master, and travis should hopefully work now.

…[] -> {} I've driveby added the require owner commit attribute as an omitmissing as it was missing from the API. Because it is omitmissing it is not a breaking change. Added spew library which helped me discover that we were comparing returned URLs from the AdminEnforcement object. None of the other objects in the API care about their URLs so I just removed that.

beiriannydd · 2017-08-27T07:35:51Z

Thanks @willnorris

beiriannydd · 2017-08-27T07:48:51Z

test/integration/repos_test.go

-				Teams: []string{},
-			},
-			DismissStaleReviews: true,
+			// The way to disable the setting is to send: &github.DismissalRestrictionsRequest{},


2017/08/27 08:08:04 updated protection: github.Protection{RequiredStatusChecks:github.RequiredStatusChecks{Strict:true, Contexts:["Pipeline"]}, RequiredPullRequestReviews:github.PullRequestReviewsEnforcement{DismissalRestrictions:github.DismissalRestrictions{}, DismissStaleReviews:true, RequireCodeOwnerReviews:false}, EnforceAdmins:github.AdminEnforcement{Enabled:true}, Restrictions:github.BranchRestrictions{Users:[], Teams:[github.Team{...}]}} 2017/08/27 08:08:04 [gh resp]: github.Response{Response:http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/1.1", ProtoMajor:1, ProtoMinor:1, Header:map[X-Ratelimit-Limit:[500... faye-salwin:delivery faye.salwin$ grep -5 DismissalRestrictionsRequest * delivery.go- RequiredStatusChecks: &github.RequiredStatusChecks{ delivery.go- Strict: true, delivery.go- Contexts: r.Contexts, delivery.go- }, delivery.go- RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{ delivery.go: DismissalRestrictionsRequest: &github.DismissalRestrictionsRequest{}, delivery.go- DismissStaleReviews: true, delivery.go- RequireCodeOwnerReviews: false, delivery.go- }, delivery.go- Restrictions: &github.BranchRestrictionsRequest{ delivery.go- Users: []string{},

This is the code I've tested the empty object with. Edited now that I added Stringify. Also now that I've added Stringify, I want to turn it off again. It really should just be used for debug ;)

dmitshur · 2017-09-05T18:20:24Z

Unfortunately, I'm finding this PR pretty hard to review and make progress on. I have a general understanding of the changes, but I don't have them completely fully grasped to be confident.

It makes many changes in important places that I'd want to discuss and confirm (e.g., changing omitemptys). There are some typos and inconsistencies in documentation that we'll need to resolve.

I wish it were smaller, but I'm not sure viable to split this PR into smaller changes (may be it is, maybe not).

@gmlewis Do you have any thoughts on this or ideas for what we can do to make it easier to make progress here?

gmlewis · 2017-09-05T19:39:54Z

I've looked at this PR and am also finding it a bit overwhelming... so I decided to look at one tiny detail - the removal of the URL field from the AdminEnforcement struct around line 612.

I found that this field was added in #610 and then looked up the API in the GitHub Developer docs and found this:
https://developer.github.com/v3/repos/branches/#get-admin-enforcement-of-protected-branch
which makes it appear as though URL is still fully supported.

So I went back through the comments of this PR and saw that the only reason for removing this field was that it made testing more difficult and that it was not seen in the wild.

At the very least, I would say that to make progress on this PR, we really need to strip out changes such as this one (that appear to be unrelated to the root issue) into separate PRs where they can be discussed individually on their own merits.

Ideally, it would be nice to have detailed instructions on the steps needed to test out these API changes... so that the maintainers can confirm that the PR changes are necessary.

(And while I'm here, I thought the whole point of Stringify was to be able to add a %v (or %s) to a format string so that the object could be printed (via its String method) so that the Stringify method did not need to be called directly... but that's a separate issue.)

beiriannydd · 2017-09-18T18:35:53Z

I'm going to close this PR, I'll continue to patch to have this functionality locally. I don't have time to go back and re-implement it in smaller pieces. Thanks for your consideration. Things to bear in mind: when the URL is returned, it is for a randomly created repository. You'll need to match this in your tests. I didn't have time to do this, but wanted passing tests to mean something. You'll notice that I did not remove any tests on the URL field. Someone re-adding URL field should also make the working test, which I think would be the right way to do this. I'm sorry that this PR is hard to follow, but that's mainly because there have been so many changes approved while this was an acceptable failure.

dmitshur · 2017-09-20T22:28:59Z

Thanks for the update @beiriannydd. I understand your position. It's completely okay to withdraw from working on a PR if it's no longer a priority for you and you're unable to take it further. This is not an easy issue to get an understanding about, to write code for, and to review.

Thank you for contributing anyway, and I'm sure the information and code available in this PR will still be useful to us or another future contributor.

We'll try to figure out how to best proceed from here.

beiriannydd mentioned this pull request Aug 21, 2017

PullRequestReviewsEnforcementRequest.DismissalRestrictionsRequest set to nil results in 422 from GH #664

Closed

beiriannydd commented Aug 21, 2017

View reviewed changes

dmitshur reviewed Aug 22, 2017

View reviewed changes

beiriannydd commented Aug 22, 2017

View reviewed changes

elliott-beach mentioned this pull request Aug 24, 2017

[#647] Add 'affiliation' to ListCollaborators. #706

Merged

beiriannydd commented Aug 24, 2017

View reviewed changes

Faye Salwin added 4 commits August 27, 2017 00:30

remove spew

67265c7

Use Stringify() and explicitly set DismissalRestrictionsRequest to nil.

247628f

Clarifications based on further testing

7859488

beiriannydd force-pushed the fix_branch_permissions_update branch from ca99fba to 7859488 Compare August 27, 2017 07:33

beiriannydd commented Aug 27, 2017

View reviewed changes

beiriannydd closed this Sep 18, 2017

dmitshur mentioned this pull request Nov 15, 2017

Bug fix PUT /repos/:owner/:repo/branches/:branch/protection #779

Closed

ethomas2 mentioned this pull request Mar 17, 2018

Fix incorrect marshalling of DismissalRestrictionsRequest field. #872

Closed

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

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

Conversation

beiriannydd commented Aug 20, 2017

Uh oh!

beiriannydd commented Aug 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Aug 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Aug 22, 2017

Uh oh!

beiriannydd commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beiriannydd Aug 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beiriannydd commented Aug 26, 2017

Uh oh!

willnorris commented Aug 27, 2017

Uh oh!

willnorris commented Aug 27, 2017

Uh oh!

beiriannydd commented Aug 27, 2017

Uh oh!

beiriannydd Aug 27, 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 commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Sep 5, 2017

Uh oh!

beiriannydd commented Sep 18, 2017

Uh oh!

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

Uh oh!

Uh oh!

dmitshur Aug 23, 2017 •

edited

Loading

beiriannydd Aug 24, 2017 •

edited

Loading

beiriannydd Aug 27, 2017 •

edited

Loading

dmitshur commented Sep 5, 2017 •

edited

Loading

dmitshur commented Sep 20, 2017 •

edited

Loading