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

Refactored setup of google api clients #2018

Merged
merged 2 commits into from
Feb 10, 2017
Merged

Conversation

freider
Copy link
Contributor

@freider freider commented Feb 6, 2017

Description

  • Adds out-of-the-box support for google cli authentication auto-discovery (get_application_default)
  • Reuses more code between gcs and bigquery implementations

Motivation and Context

Works with the current version of the cli tools and api client (google-api-python-client==1.6.1), which was previously not working (http and oauth could not both be provided as was the default/required behavior by the previous code).

##Tests
I ran tasks both for gcs (cloud storage) uploads and big query integration and both work after this patch. I don't see how it would break anything for people who use manual configuration of the authentication process, but it would of course be nice if someone who has a GCS/BigQuery pipeline running tested this out as well :)

@Tarrasch
Copy link
Contributor

Tarrasch commented Feb 9, 2017

Oh, are you back at Spotify now? :)

There's also luigi tests for gcs which I recall are disabled on Travis. Have you run them too?

@freider
Copy link
Contributor Author

freider commented Feb 9, 2017

Hi Arash! :-)
Not back at Spotify but back in Luigi land at least part-time as we're setting up some simple log processing at EdQu using BigQuery - so I can motivate spending more some time on luigi again. It's cool to see all the new stuff that's has been contributed.
Ah, I'll have a look and run the gcs tests locally to verify that it still works.

Reuses more code between gcs and bigquery implementations. Adds out-of-the-box
support for google cli authentication auto-discovery with the current version
of the cli tools, which was previously not working (http and oauth could not
both be provided as was the default behavior)
@freider
Copy link
Contributor Author

freider commented Feb 9, 2017

I ran the tests gcs and bigquery tests with success (pre-patch they would not run using the current version of google-api-python-client (1.6.2)).
Noteworthy is that the tests won't run using python3 as there seem to be some issues there unrelated to this patch - guessing that code has never been tested on py3.

Any idea what is causing the travis fail for this PR? It just looks like jibberish to me :-)

@Tarrasch
Copy link
Contributor

Tarrasch commented Feb 9, 2017

Yea the hadoop failures are jibberish. But the flake8 issue you certainly should be able to address. Here's you log, https://travis-ci.org/spotify/luigi/jobs/198885463. There's some docs in CONTRIBUTING.md too nowadays. :)

@freider
Copy link
Contributor Author

freider commented Feb 9, 2017

Ah, should have seen that, thanks! :)

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe someone using gcp wants to review too?

@freider
Copy link
Contributor Author

freider commented Feb 10, 2017

I wonder if many other use the gcp integration actively, considering the code prior to this patch doesn't work with the current google-api-python-client version... (so if they use it they probably use an old version and wouldn't upgrade luigi either ;) )

@Tarrasch
Copy link
Contributor

@freider, I'm pretty sure there's others using it. Even Spotify is contributing to it still and I've seen various parties commenting to PRs, that said, nobody have commented for a while so I'll just merge this. :)

@Tarrasch Tarrasch merged commit fba731a into spotify:master Feb 10, 2017
kreczko pushed a commit to kreczko/luigi that referenced this pull request Mar 28, 2017
Reuses more code between gcs and bigquery implementations. Adds out-of-the-box
support for google cli authentication auto-discovery with the current version
of the cli tools, which was previously not working (http and oauth could not
both be provided as was the default behavior)
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.

2 participants