Skip to content

Remove oauth2client._helpers dependency - #493

Merged
theacodes merged 3 commits into
googleapis:masterfrom
helen-fornazier:no-oauth
Apr 26, 2018
Merged

Remove oauth2client._helpers dependency#493
theacodes merged 3 commits into
googleapis:masterfrom
helen-fornazier:no-oauth

Conversation

@helen-fornazier

Copy link
Copy Markdown

oauth2client is deprecated
Migrate required helper's code from oauth2client to googleapiclient to
decrease our dependency on oauth2client

_helpers.py copied from oauth2client code
https://github.com/google/oauth2client/blob/master/oauth2client/_helpers.py

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 20, 2018
@helen-fornazier

helen-fornazier commented Apr 20, 2018

Copy link
Copy Markdown
Author

Please review, I am not entirely sure if this is the appropriated solution, please let me know what you think (I'll fix the errors in tox, I just would like some feedback if this is ok).
With this change plus #492 I can already manipulate instances using just google-auth, after uninstalling oauth2client

More specifically, what I am trying to run with google-auth instead of oauth2client is:
[code]: https://github.com/ClusterLabs/fence-agents/blob/master/fence/agents/gce/fence_gce.py
[PR]: ClusterLabs/fence-agents#197

Thanks

@theacodes

Copy link
Copy Markdown
Contributor

This is actually great, @helen-fornazier! Could you also copy over the tests for those helpers from oauth2client?

@helen-fornazier

Copy link
Copy Markdown
Author

@jonparrott done, I imported the tests too as suggested. Could you please take a look in the commits?

Thanks

@helen-fornazier

Copy link
Copy Markdown
Author

Also, now the only file inside googleapiclient/ who uses oauth2client is googleapiclient/sample_tools.py, do you know if this module is used by external libraries?
It seems to me that this file is just an utility module used by samples/, so I was wondering if we could move sample_tools.pyto the samples/ folder and completely remove oauth2client from the install_requires list in the setup.py

@theacodes

Copy link
Copy Markdown
Contributor

We can't remove sample_tools.py without causing a major-version breaking change, unfortunately. We can make it be helpful if oauth2client isn't available, something like this:

try:
    import oauth2cient
except:
    raise ImportError('googleapiclient.sample_tools requires oauth2client. Please install oauth2client and try again.')

Helen Koike added 2 commits April 26, 2018 14:45
oauth2client is deprecated
Migrate required helper's code from oauth2client to googleapiclient to
decrease our dependency on oauth2client

_helpers.py copied from oauth2client code
https://github.com/google/oauth2client/blob/master/oauth2client/_helpers.py
oauth2client is deprecated
Migrate required helper's code from oauth2client to googleapiclient to
decrease our dependency on oauth2client

test__helpers.py copied from oauth2client code
https://github.com/google/oauth2client/blob/master/tests/test__helpers.py
@helen-fornazier

Copy link
Copy Markdown
Author

@jonparrott lgtm, I added your suggestion in the PR, could you take a look please?

Can I remove oauth2client from the install_requires list or replace it by google-auth? https://github.com/google/google-api-python-client/blob/master/setup.py#L67

@theacodes theacodes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one tiny nit.

Comment thread googleapiclient/sample_tools.py Outdated
from oauth2client import client
from oauth2client import file
from oauth2client import tools
except:

This comment was marked as spam.

This comment was marked as spam.

@theacodes

theacodes commented Apr 26, 2018

Copy link
Copy Markdown
Contributor

Can I remove oauth2client from the install_requires list or replace it by google-auth?

Yes, but let's do it in two releases: one to have this, and then a second one to remove oauth2client.

oauth2client shouldn't be mandatory for the googleapiclient.
Add a try except statement around importing oauth2client

Suggested-by: Jon Wayne Parrott
@theacodes
theacodes merged commit de13e3b into googleapis:master Apr 26, 2018
@theacodes

Copy link
Copy Markdown
Contributor

@helen-fornazier thank you so much for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants