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 client to accommodate oauth2client>=4.0 #184

Merged
merged 1 commit into from
Nov 16, 2017
Merged

Update client to accommodate oauth2client>=4.0 #184

merged 1 commit into from
Nov 16, 2017

Conversation

eap
Copy link
Contributor

@eap eap commented Nov 15, 2017

This changes has been needed for a while now. The main blocker
seems to be the use of locked_file for caching GCE credentials.
I've added a simple multiprocess lockable file cache that uses
a similar approach to that used in ouath2client's multiprocess
file storage.

Submission of this should close issue #162.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.086% when pulling 518c95c on e4p:master into 4220ed9 on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.084% when pulling 76fd06b on e4p:master into 4220ed9 on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.07% when pulling 5d02993 on e4p:master into 4220ed9 on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.07% when pulling 391253e on e4p:master into 4220ed9 on google:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.068% when pulling 540b50f on e4p:master into 4220ed9 on google:master.

self._GetServiceCreds(service_account_name='my_service_account',
scopes=scopes)
self._GetServiceCreds(service_account_name=None,
scopes=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did the linter not complain about the odd spacing here (and the statement below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did, I've added a fix in the latest test. How about I squash this all

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. That's the only nit I had -- LGTM if you're addressing that :)

This changes has been needed for a while now. The main blocker
seems to be the use of locked_file for caching GCE credentials.
I've added a simple multiprocess lockable file cache that uses
a similar approach to that used in ouath2client's multiprocess
file storage.

Submission of this should close issue #162.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.068% when pulling 0ef72f6 on e4p:master into 4220ed9 on google:master.

@eap
Copy link
Contributor Author

eap commented Nov 15, 2017

I ran the gsutil integration tests, both on a clean clone of gsutil, and on a clone where I substituted my apitools repo into the third_party directory. The change introduced no new failures (although 2 unit tests and one integration test seemed to be already failing in the unaltered repo).

@houglum
Copy link
Contributor

houglum commented Nov 16, 2017

Just ran the gsutil integration tests with this apitools fork against:

  • A fresh clone of gsutil on an Ubuntu machine, removing third_party/apitools and copying in e4p's fork into its place -- all tests passed.
  • The same code, but run sequentially on a Windows 7 machine -- all passed there as well.

@craigcitro craigcitro merged commit 63e97c1 into google:master Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants