Skip to content
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.

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

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

Closed
mslusarczyk opened this issue Jul 3, 2017 · 8 comments · Fixed by #791

Comments

@mslusarczyk
Copy link

According to doc user is able to pass nil to disable DismissalRestrictions and looking at method below this nil is converted to empty array.

Seems that this is not working properly because

&github.ProtectionRequest{
		RequiredPullRequestReviews: &github.PullRequestReviewsEnforcementRequest{DismissalRestrictionsRequest: nil, DismissStaleReviews:false},
	}

results in:

No subschema in "anyOf" matched.
For 'properties/dismissal_restrictions', [] is not an object.
Not all subschemas of "allOf" matched.
For 'anyOf/1', {"dismissal_restrictions"=>[], "dismiss_stale_reviews"=>false} is not a null. []]

and '422 Invalid request' from GH

@navarrano could you please take a look?

@dmitshur dmitshur added the bug label Jul 4, 2017
@iwata
Copy link

iwata commented Jul 21, 2017

And I hope that PullRequestReviewsEnforcementRequest supports require_code_owner_reviews key 😄 .
https://developer.github.com/v3/repos/branches/#update-branch-protection

@dmitshur
Copy link
Member

dmitshur commented Jul 21, 2017

I ran into this issue as well in #674. See

// TODO: This test fails with 422 Validation Failed [{Resource: Field: Code: Message:}].
// Someone familiar with protection requests needs to come up with
// a valid protection request that doesn't give 422 error.
.

When this issue is resolved, that integration test can be fixed too.

beiriannydd pushed a commit to beiriannydd/go-github that referenced this issue Aug 10, 2017
beiriannydd pushed a commit to beiriannydd/go-github that referenced this issue Aug 10, 2017
…ionsRequest set to nil results in 422 from GH
beiriannydd pushed a commit to beiriannydd/go-github that referenced this issue Aug 20, 2017
…[] -> {}

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
Copy link

I have PR #703 in to fix this. It seems to be failing because I added a new library go-spew which is essentially the recursive %+v I always wanted and that doesn't get pulled in during the build.

@beiriannydd
Copy link

Tests are passing :)

@dmitshur
Copy link
Member

See my comment at #703 (comment).

I think in order to resolve this bug, we need to figure out how to reproduce a successful response from GitHub using REST API. Then, once we have that, we can work on the Go code to fix the code, if needed.

beiriannydd pushed a commit to beiriannydd/go-github that referenced this issue Aug 27, 2017
…[] -> {}

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.
@dmitshur
Copy link
Member

dmitshur commented Sep 5, 2017

From my understanding of #703, the way to get a successful response from GitHub is to perform the API call involving DismissalRestrictionsRequest on an organization-owned repository. It doesn't work on personal repositories and gives 422 in that case. /cc @beiriannydd

@JaredReisinger
Copy link

JaredReisinger commented Oct 30, 2017

I just started seeing a 422 when an empty array is passed for dismissal_restrictions. The docs (https://developer.github.com/v3/repos/branches/#update-branch-protection) specifically mention that passing an empty array is the right way to disable dismissal restrictions (which is weird, IMO, since null would work just fine for this, and the object isn't normally an array)... so the current implementation is as per the docs, but GitHub still seems to be complaining.

In any case, I have a support request out to GitHub to try to find out what the expected behavior is/should be. If the docs are wrong, and the property should be null (instead of an empty array), I'll provide a PR to make the change.

Update: I just tried passing null—by modifying the JSON Marshal code—and GitHub complains about that too... the actual problem might be on GitHub's end. (I'm attempting to modify a private organization repo, not a personal or public repo.)

@dmitshur
Copy link
Member

dmitshur commented Mar 19, 2018

It seems to me that GitHub has updated the docs by now, and I'm not aware of what exactly we're waiting on them for, so removed that label. I think PR #791 will resolve this issue (let me know if not).

mspiegel added a commit to capitalone/checks-out that referenced this issue Mar 28, 2018
Fixes google/go-github#664
id type changed from int to int64
mspiegel added a commit to capitalone/checks-out that referenced this issue Mar 28, 2018
Fixes google/go-github#664
id type changed from int to int64
mspiegel added a commit to capitalone/checks-out that referenced this issue Mar 28, 2018
- Fixes google/go-github#664
- id type changed from int to int64
nbareil pushed a commit to nbareil/go-github that referenced this issue May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants