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

AIRFLOW-15: Remove gcloud #1448

Merged
merged 1 commit into from
Apr 28, 2016
Merged

AIRFLOW-15: Remove gcloud #1448

merged 1 commit into from
Apr 28, 2016

Conversation

criccomini
Copy link
Contributor

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.25% when pulling 1218120 on criccomini:remove-gcloud into 6cbe4a4 on airbnb:master.

@criccomini criccomini closed this Apr 28, 2016
@criccomini criccomini reopened this Apr 28, 2016
@criccomini
Copy link
Contributor Author

Test failure is due to deletion of some files, which reduces license header count.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.18% when pulling c2dc266 on criccomini:remove-gcloud into 6cbe4a4 on airbnb:master.

@criccomini
Copy link
Contributor Author

I added some license headers to make the tests pass.

@coveralls
Copy link

coveralls commented Apr 28, 2016

Coverage Status

Changes Unknown when pulling c2dc2665a2fa56b5d4c5d276794aa59dbe2110f4 on criccomini:remove-gcloud into * on airbnb:master*.

# upload from within the file context (it hasn't been
# closed).
tmpfile.flush()
self.hook.upload(bkt, blob, tmpfile.name)
except:
pass
Copy link
Member

Choose a reason for hiding this comment

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

I think the logging.error() that is below this line (not shown in github diff) should now go inside this except statement. Otherwise it prints the error every time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Fixing.

@jlowin
Copy link
Member

jlowin commented Apr 28, 2016

Please see my one comment about the error always printing. Also, I tried running this in my local env with no key in the Airflow connection (in other words, using global gcloud auth) and got the following message:

INFO:root:Getting connection using `gcloud auth` user, since no service_account/key_path are defined for hook.
WARNING:oauth2client.util:__init__() takes at most 2 positional arguments (3 given)

But I'm not sure if this matters (or even if it's on the Airflow side), since it appears to read/write the logs to GCS just fine.

One last thought -- would be great to bump this up to oauth2client 2.0 compatibility. Maybe not this PR but at some point. This is the offending line: from oauth2client.client import SignedJwtAssertionCredentials, GoogleCredentials. I think the signed credentials just need to be pulled from a different module but I don't know the library well so I'm not sure.

@jlowin
Copy link
Member

jlowin commented Apr 28, 2016

I just saw your message on the Apache issue that you're already looking at compatibility issues :) Sorry for being repetitive. Need to get used to the split infrastructure...

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.18% when pulling 844eb2c on criccomini:remove-gcloud into 6cbe4a4 on airbnb:master.

@coveralls
Copy link

coveralls commented Apr 28, 2016

Coverage Status

Changes Unknown when pulling 844eb2c on criccomini:remove-gcloud into * on airbnb:master*.

@criccomini
Copy link
Contributor Author

@jlowin updated based on your feedback.

Could I get a +1?

Re: oauth compatibility, I'll bump to latest as part of https://issues.apache.org/jira/browse/AIRFLOW-16

@jlowin
Copy link
Member

jlowin commented Apr 28, 2016

Looking good to me @criccomini. Merge away.

@criccomini criccomini merged commit 557206c into apache:master Apr 28, 2016
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.

4 participants