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

Update applications API use to pass access token as body param instead of path param #1203

Merged
merged 3 commits into from
Mar 2, 2020
Merged

Update applications API use to pass access token as body param instead of path param #1203

merged 3 commits into from
Mar 2, 2020

Conversation

oreoshake
Copy link
Contributor

See https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api and https://developer.github.com/changes/2020-02-14-deprecating-oauth-app-endpoint/

This PR updates the calls to the applications API calls. The current implementation uses deprecated endpoint, the new implementation uses the supported, but preview endpoints.

I've got the tests passing but I've yet to actually try this against the live API 😄

@tarebyte tarebyte merged commit 6d6078a into octokit:4-stable Mar 2, 2020
key = opts.delete(:client_id) || client_id
secret = opts.delete(:client_secret) || client_secret

as_app(key, secret) do |app_client|
app_client.get "applications/#{client_id}/tokens/#{token}", opts
app_client.post "applications/#{client_id}/tokens", opts
Copy link

@augustocravosilva augustocravosilva Mar 25, 2020

Choose a reason for hiding this comment

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

Hey @oreoshake, @tarebyte,
The paths you are using here and returing 404. According to https://developer.github.com/changes/2020-02-14-deprecating-oauth-app-endpoint/#endpoints-affected , it should be singular token, and not tokens

Copy link
Member

Choose a reason for hiding this comment

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

Damn, thanks for the heads up. I'm working on a PR to resolve this.

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've got the tests passing but I've yet to actually try this against the live API 😄

🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Thanks for the quick fix. Seems to be working correctly now :)

@emmahsax
Copy link

@tarebyte Does this change mean that the newest version of Octokit will address this: https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/?

@tarebyte
Copy link
Member

Does this change mean that the newest version of Octokit will address this: https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/?

Yup that should be it!

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.

4 participants