Use updated metadata auth call credentials core API in Python (and migrate to Cython)#4228
Conversation
There was a problem hiding this comment.
Thanks. I take it that the API change will be additive (non breaking) when we add the auth_context.
There was a problem hiding this comment.
I believe it'll be additive.
There was a problem hiding this comment.
nit: call credentials -> CallCredentials
There was a problem hiding this comment.
The return type itself is in the return type doc-comment below, and I'd feel a bit weird then calling the auth metadata plugin an 'auth metadata plugin' as opposed to giving it some proper type.
Actually, that's a question: should the metadata_callback argument actually be some metadata_auth_plugin argument with an abstract class interface somewhere? I'm leaning towards no, for ease of use, but just bringing it up as a possibility.
There was a problem hiding this comment.
The return type itself is in the return type doc-comment below, and I'd feel a bit weird then calling the auth metadata plugin an 'auth metadata plugin' as opposed to giving it some proper type.
I'm OK with that but then, I think it would make sense to change the doc for the ssl_credentials which uses ChannelCredentials as opposed to channel credentials (see my new comment on this).
Actually, that's a question: should the metadata_callback argument actually be some metadata_auth_plugin argument with an abstract class interface somewhere? I'm leaning towards no, for ease of use, but just bringing it up as a possibility.
This more of a python question I believe. What would you, user of the API, think would be the most appropriate? Maybe @nathanielmanistaatgoogle has some good insight on this.
There was a problem hiding this comment.
Yes, it should - I'm really stumbling over the complicated type description here.
|
Changes LGTM. Thanks much! After you have addressed the latest cosmetic comments, can you please do a PR to update the doc as well? Here's the C# one for a model: |
There was a problem hiding this comment.
So maybe ChannelCredentials -> channel credentials here to be consistent with call credentials below?
There was a problem hiding this comment.
With the changed interface choices, went in the other direction with the below.
eddd6ec to
7430062
Compare
|
Probably-irrelevant fact: GitHub is currently showing the commits in this pull request to me as: |
|
Looks mostly fantastic; one major comment left on soltanmm@2c6cf12 and a few minor comments sprinkled around. |
d42169c to
cffade9
Compare
|
Rebased. |
|
Interop tests will fail on this until #4323 goes in. |
|
(interop test failures are unrelated, unit tests are green) |
|
Thanks much! I'm good with this. @nathanielmanistaatgoogle do you want to merge? |
|
Probably-irrelevant-but-again-fun fact: GitHub is again showing these commits to me in a crazy order and I again had to draw them out on paper to persuade myself that they represent a linear sequence of development. GitHub is showing them to me from top to bottom as soltanmm@f21dc7e . If we semantically order the commits as 1,2,3,4,5,6 (and "parents much precede children" semantic ordering is clear regardless of the chronological author timestamp and commit timestamp of any commit), then the order in which GitHub is showing them in this code review is 3,1,6,5,4,2.
|
|
I'm seeing that, too... so weird. |
|
Change looks good to me in all respects but test results. @soltanmm are the test results I'm currently seeing all "explained" and this is safe to merge? Should anything be rerun? |
|
All test failures are in Ruby and cannot AFAIK mask possible Python failures. |
Use updated metadata auth call credentials core API in Python (and migrate to Cython).
Arguably some Python casing conventions aren't being followed in the
records.pyx, but they were there before (... and I disagree with myself now...) and we can go back and change them before GA anyway.@jboeuf see files src/python/grpcio/grpc/beta/implementations.py and src/python/grpcio/grpc/beta/interfaces.py
Fixes #3908
cc @nathanielmanistaatgoogle
cc #282