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

Replace oauth2client by google-auth #2361

Merged
merged 4 commits into from
Mar 5, 2018
Merged

Replace oauth2client by google-auth #2361

merged 4 commits into from
Mar 5, 2018

Conversation

tmattio
Copy link
Contributor

@tmattio tmattio commented Feb 22, 2018

As mentioned in https://github.com/google/oauth2client, oauth2client is now deprecated and Google recommends using google-auth (https://github.com/GoogleCloudPlatform/google-auth-library-python).

This PR replaces all usages of oauth2client bygoogle-auth

Description

oauth2client was only used to get the application default login. The PR replaces the call to the equivalent call in google-auth, and changes the documentation to match.

Motivation and Context

I had problems using Luigi with the latest version of oauth2client. Rather than downgrading, I decided to update the code.

Have you tested this? If so, how?

I am using the contrib modules gcs and gcp in production. I have no use of the other modules, however, and did not test the changes in those. The changes in the other modules are exactly the same as gcs and gcp, but I would really appreciate it if someone who makes use of these modules could confirm that nothing broke!

@Tarrasch
Copy link
Contributor

This PR is changing a module I don't know so much about. Can you do a git blame and look for other contributors who have edited those files and @-mention them to summon them as reviewers of this patch? Once others +1. I can help press "merge".

@tmattio
Copy link
Contributor Author

tmattio commented Feb 22, 2018

@constantijn @freider @mikekap Does it seem to make sense to you?

Thanks!

@himikof
Copy link
Contributor

himikof commented Feb 23, 2018

While it is not directly related, maybe googleapis/google-api-python-client#299 could be handled at the same time (because it touches the very same codepath)? It should be as simple as passing cache_discovery=False when performing discovery.build(). This issue leads to a loud log noise without an old oauth2client version installed (which is about to become even more common after this change).

@constantijn
Copy link

constantijn commented Feb 26, 2018

The Dataproc plugin looks good to me. If the tests work then +1 for me.

@mikekap
Copy link
Contributor

mikekap commented Feb 26, 2018

My only concern is: given there’s no flavors of Luigi, this is effectively a breaking change. Would it make sense to fall back to oauth2client with a loud warning if the new lib isn’t installed?

@dlstadther
Copy link
Collaborator

Is there any user-required action other than installing the correct library?

If not, i'd say force the change and make a huge note of it in the release notes for 2.7.3 (or whichever next version after this PR is merged).

@constantijn
Copy link

They're all in try: except ImportError: blocks. If you're not using any google cloud specific tasks you won't even notice.

@tmattio
Copy link
Contributor Author

tmattio commented Mar 1, 2018

@dlstadther No, there is no user-required action. The two API calls do the same thing, that is try and get the current gcloud application default login on the system.

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

LGTM

@honnix
Copy link
Member

honnix commented Apr 4, 2018

Not entirely sure, but it seems https://github.com/google/google-api-python-client/blob/master/googleapiclient/_auth.py#L95 would fail due to missing google_auth_httplib2 which is not a dependency of Luigi.

Found the following issue when using luigi.contrib.BigQueryTarget:

 File "/usr/local/lib/python2.7/dist-packages/luigi/contrib/bigquery.py", line 394, in __init__
    self.client = client or BigQueryClient()
  File "/usr/local/lib/python2.7/dist-packages/luigi/contrib/bigquery.py", line 129, in __init__
    self.client = discovery.build('bigquery', 'v2', cache_discovery=False, **authenticate_kwargs)
  File "/usr/local/lib/python2.7/dist-packages/oauth2client/util.py", line 137, in positional_wrapper
    return wrapped(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/googleapiclient/discovery.py", line 232, in build
    credentials=credentials)
  File "/usr/local/lib/python2.7/dist-packages/oauth2client/util.py", line 137, in positional_wrapper
    return wrapped(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/googleapiclient/discovery.py", line 371, in build_from_document
    http = _auth.authorized_http(credentials)
  File "/usr/local/lib/python2.7/dist-packages/googleapiclient/_auth.py", line 97, in authorized_http
    'Credentials from google.auth specified, but '
ValueError: Credentials from google.auth specified, but google-api-python-client is unable to use these credentials unless google-auth-httplib2 is installed. Please install google-auth-httplib2.

honnix added a commit that referenced this pull request Apr 4, 2018
This was referenced Jun 29, 2022
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.

7 participants