-
Notifications
You must be signed in to change notification settings - Fork 110
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
Upgrading oauth2client.tools.run to run_flow. #13
Conversation
2 similar comments
yeah, i can't just drop the only option here is the (awkward) "try |
Nope, there is a "better" option that let's you move off of flags = argparse.Namespace(auth_host_name=FLAGS.auth_host_name,
auth_host_port=FLAGS.auth_host_port,
logging_level='ERROR',
noauth_local_webserver=not FLAGS.auth_local_webserver) |
ah, so you're saying we can drop |
@craigcitro PTAL. I made the changes to incorporate |
@@ -13,7 +14,9 @@ | |||
import oauth2client.locked_file | |||
import oauth2client.multistore_file | |||
import oauth2client.service_account | |||
import oauth2client.tools # for flag declarations | |||
# The `tools` import is rarely needed, but takes a long time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craigcitro I ported this comment from the actual changed section below. It seems to be out-of-date (i.e. the slow import is just oauth2client.crypt
or anything that imports it) but I left it in just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i vaguely remember running into this and not digging much. you can probably drop this whole comment now (since we use tools
for the arg parser).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still needed for the gflags
declarations because it imports old_run
. The "long time" thing is what I was getting hung up on.
LGTM, one or two little comments/questions |
Yup. I've got 4 changes to make and one remaining question for you (about |
@craigcitro I pushed over the original commit with the changes. Main update is just checking for each attribute on |
@@ -13,7 +14,8 @@ | |||
import oauth2client.locked_file | |||
import oauth2client.multistore_file | |||
import oauth2client.service_account | |||
import oauth2client.tools # for flag declarations | |||
# It is needed for gflags declarations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It
is ambiguous (is it the next declaration or the previous one?). maybe either name the module or keep it on the same line?
looks great -- one really nitpicky comment and then we're good to go. 👍 |
Allowing fallback to FLAGS within this library without having to rely on gflags within `oauth2client`.
Yeah I noticed the ambiguous Addressed and pushed over the original commit. |
Upgrading oauth2client.tools.run to run_flow.
@craigcitro I assume this won't be sufficient / you need some legacy support for
gflags
.NOTE: I reference the warning for the out-of-date method in googleapis/google-cloud-python#755
I tested this with
and it works just fine.