Refactor auth code to ease transition to google-auth #135
Conversation
|
/cc @bjwatson |
Current coverage is 98.21% (diff: 100%)@@ master #135 diff @@
==========================================
Files 8 8
Lines 616 616
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 604 605 +1
+ Misses 12 11 -1
Partials 0 0
|
| ignored. | ||
| service_path: The domain name of the API remote host. | ||
| service_port: The port on which to connect to the remote host. | ||
| credentials: The credentials for this request. |
There was a problem hiding this comment.
Could you clarify the difference between credentials and ssl_credentials in the docstring?
| @@ -31,19 +31,49 @@ | |||
|
|
|||
| from __future__ import absolute_import | |||
|
|
|||
| import grpc | |||
There was a problem hiding this comment.
There is a design restriction on gax not to import grpc in any module except google.gax.grpc. (The intention is that when we support alternative transports, we should only need to switch out a single module.) Let's move secure_authorized_channel to grpc for this reason. gax should also not expose any methods that are grpc-specific in its public interface, so let's make secure_authorized_channel "private".
There was a problem hiding this comment.
So this is here because eventually I'll defer most of this to google-auth - (see this PR - you'll notice a similar secure_authorized_channel function).
I can move this into grpc, but at that point I might as well just delete the auth file altogether - it will provide no discernible value. Which would you like?
There was a problem hiding this comment.
Actually - I think I'm going to shove this into _grpc_oauth2client.py, so that I can add _grpc_google_auth.py with the same interface.
There was a problem hiding this comment.
That makes sense. Let's just get rid of the auth module altogether. If I recall correctly, we only created it to begin with because oauth2client didn't provide the generic auth function; since google-auth is going to do that, we shouldn't expose the functionality on the gax surface.
There was a problem hiding this comment.
Sorry, posted that comment before I saw yours. Moving it to a private module SGTM.
| _fake_create_stub, self.FAKE_SERVICE_PATH, self.FAKE_PORT, | ||
| metadata_transformer=lambda x: tuple()) | ||
| self.assertFalse(auth.called) | ||
| self.assertFalse(secure_authorized_channel.called) |
There was a problem hiding this comment.
You should pass these variables as parameters to grpc.create_stub; otherwise this doesn't actually test anything.
There was a problem hiding this comment.
I'm not sure what you mean here?
There was a problem hiding this comment.
Still not sure what you mean here.
There was a problem hiding this comment.
Sorry, I misread this. Disregard this comment.
| _fake_create_stub, service_path=self.FAKE_SERVICE_PATH, | ||
| service_port=self.FAKE_PORT, credentials=credentials) | ||
|
|
||
| self.assertFalse(get_default_credentials.called) |
There was a problem hiding this comment.
get_default_credentials is an unused variable, so a meaningless assertion?
There was a problem hiding this comment.
This ensures that default credentials are not used when explicit credentials are specified.
There was a problem hiding this comment.
You haven't executed any code that references get_default_credentials at the point that you make the assertion that it hasn't been called; it's clear from the syntax alone that it hasn't been called. This is only meaningful if you pass it somehow to grpc.create_stub as a parameter.
There was a problem hiding this comment.
create_stub has a branch that calls get_default_credentials, this assert ensures that branch was not executed.
There was a problem hiding this comment.
Sorry, I misread this. Disregard this comment.
There was a problem hiding this comment.
:) I thought I had caught the dumb for second. :D
| from grpc import RpcError, StatusCode | ||
| from . import auth | ||
| from . import _grpc_oauth2client |
There was a problem hiding this comment.
think this file accidentally got left out of the commit
There was a problem hiding this comment.
Yeah, fixed. I also did away with the use of closures and went with @dhermes' suggestion of using a class like we do in google-auth.
| @@ -0,0 +1,94 @@ | |||
| # Copyright 2015, Google Inc. | |||
There was a problem hiding this comment.
Cool. So this file goes away entirely once googleapis/google-auth-library-python#67 is merged, right?
There was a problem hiding this comment.
It'll stay around until we formally deprecate oauth2client. I'll also be adding another module named _grpc_google_auth.py that'll be a very small interface to google.auth.
Done. |
|
Travis is green. ✨ |
|
Thanks @geigerj! Can I get your review over at googleapis/google-auth-library-python#67? Once we merge that I can send another PR for |
|
Will do! |
note This changes several public API methods.