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

Adds usable auth, taking a dependency on the google-auth-library repo. #374

Merged
merged 9 commits into from
Mar 10, 2015

Conversation

jasonall
Copy link
Contributor

No description provided.

@ryanseys
Copy link
Contributor

A bunch of newlines at the ends of files are missing, please fix those.

Also, what's the strategy on testing these changes? Seems a lot of tests were removed and I'm assuming moved to the other library we now depend on, however we should probably test that certain objects and functions are still exposed to the user and that all available auth objects still exist as they should. Any thoughts on that?

@jasonall
Copy link
Contributor Author

I added newlines to all the files where they were missing.

Good question about testing. There are still some auth tests within the test.oauth2.cs file. For example the 'should refresh if have refresh token but no access token' test spins up a Drive API, sets a refresh token, and makes sure that the auth library kicks in and fetches a refresh token before executing the API.

@ryanseys
Copy link
Contributor

Yeah, I noticed you didn't rip them all out which is good, and obviously it doesn't make sense to have them all stay. As suggested, it might be just good to ensure that all the objects we expect to be there are there, anything we have documented in our README (JWT, OAuth2, etc) should likely be tested to exist.

Might also be good to recruit someone working on auth stuff to code review this thoroughly or at least give it a thumbs up and I'll merge. I'm maintaining this repo as a previous intern / future full-timer in my spare time :) so I'm unsure what the timeline for these changes are. Overall looks really good (I like all the deletions), though I haven't had a lot of time to go deep down into the auth repo.

@jasonall
Copy link
Contributor Author

I added some new tests to ensure the existence of the jwt and compute types. The other types are covered already.

Thanks.

@tbetbetbe
Copy link
Contributor

Please re-sync the PR as at the moment, it can't be merged.

@tbetbetbe
Copy link
Contributor

Missing: package.json should be updated

  • switch the version from 1.1.5 to 2.0.0
    • this follows semantic version strictly
    • is necessary behaviour has moved out of this package into google-auth-library
  • update to the list of maintainers to add @jasonall and @tbetbetbe (and also remove @monsur)

@jasonall
Copy link
Contributor Author

jasonall commented Mar 9, 2015

Re-synced the PR, merged in the latest changes, merged in those changes to the google-nodejs-auth-library repo (where they now belong), removed the changes from this repo, updated the package version, and updated the list of maintainers.

tbetbetbe added a commit that referenced this pull request Mar 10, 2015
Adds usable auth, taking a dependency on the google-auth-library repo.
@tbetbetbe tbetbetbe merged commit 32660eb into googleapis:master Mar 10, 2015
@ryanseys
Copy link
Contributor

Woohoo! Congrats on shipping! :) Can you document the changes in the GitHub release? https://github.com/google/google-api-nodejs-client/releases/tag/v2.0.0

Also we have a migration guide from 0.x to 1.x so now documenting going to 2.x might be appropriate with the auth related changes that are needed. Either that or remove the old migration guide because it might be confusing to users now using the 2.x library.

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.

3 participants