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

Remove the :name key from #add_team_repository request #1054

Merged
merged 1 commit into from
Aug 16, 2018
Merged

Remove the :name key from #add_team_repository request #1054

merged 1 commit into from
Aug 16, 2018

Conversation

BenEmdon
Copy link
Contributor

@BenEmdon BenEmdon commented Aug 16, 2018

What

It appears the API began validating the presence of parameters on the request teams/:team_id/repos/:org/:repo_name. When a request contained the parameter key name it would be rejected by the API with a status of 422.

Using the method #add_team_repository would yield a 422 status with the message:

"name" is not a permitted key. // See: https://developer.github.com/v3/teams/#add-or-update-team-repository

This PR proposes a fix to drop the extra param :name which no longer serves a purpose.

API reference https://developer.github.com/v3/teams/#add-or-update-team-repository


Fixes #1051 closes #1053

@d12
Copy link

d12 commented Aug 16, 2018

@kytrinyx Do you think you could take a peek at this when you get a moment?

@BenEmdon
Copy link
Contributor Author

BenEmdon commented Aug 16, 2018

To check that this method is broken I made a PR with a failing test, then patched classroom with my octokit.rb fork.

See github-education-resources/classroom#1483

@kytrinyx
Copy link
Contributor

This is my bad. I meant to deploy this validation as a scientist experiment to see what would fail without actually making things fail. I will deploy a fix momentarily.

@BenEmdon
Copy link
Contributor Author

@kytrinyx no problem, thanks for the fix upstream!
Should this patch still merge if it is equivalent to the old way (but fitting within the ideals of upstream)?

@kytrinyx
Copy link
Contributor

I think we should merge the patch. The API doesn't accept a :name parameter according to the docs:
https://developer.github.com/v3/teams/#add-or-update-team-repository

The codebase just ignores it currently, (well, it did until two days ago /facepalm). So while we should make the API backwards compatible, it would make sense to update the client to actually match the documented behavior.

Copy link
Contributor

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

🍰

@tarebyte tarebyte merged commit 3eb1e97 into octokit:master Aug 16, 2018
@BenEmdon BenEmdon deleted the fix-add_team_repository branch August 16, 2018 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add_team_repo call now gets 422 from GitHub for "name" is not a permitted key.
4 participants