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.

Sign up for GitHub

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

Jump to bottom

Add grpc transport #67

Merged
merged 4 commits into from
Nov 3, 2016
Merged

Add grpc transport #67

merged 4 commits into from
Nov 3, 2016

Conversation

theacodes
Copy link
Contributor

@bjwatson @geigerj please review, as this will replace all of gax.auth and most of gax.grpc.

@nathanielmanistaatgoogle PTAL to verify grpc usage.

@theacodes theacodes added this to the 1.0.0 milestone Oct 31, 2016
nathanielmanistaatgoogle
nathanielmanistaatgoogle suggested changes Oct 31, 2016
View reviewed changes
google/auth/transport/grpc.py
composite_credentials = grpc.composite_channel_credentials(
ssl_credentials, google_auth_credentials)

channel = grpc.secure_channel(target, composite_credentials)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tests/transport/test_grpc.py

import mock


This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

google/auth/transport/grpc.py
google_auth_credentials = grpc.metadata_call_credentials(
google_auth_credentials_metadata_plugin)

if not ssl_credentials:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@theacodes
Copy link
Contributor Author

@nathanielmanistaatgoogle any idea why grpc.AuthMetadataPlugin doesn't exist on Python 2.7 but does for 3.4/3.5?

@theacodes
Copy link
Contributor Author

@nathanielmanistaatgoogle any idea why grpc.AuthMetadataPlugin doesn't exist on Python 2.7 but does for 3.4/3.5?

It might actually be that grpc isn't being installed. Do you know why this would be the case?

dhermes
dhermes approved these changes Oct 31, 2016
View reviewed changes
docs/conf.py
@@ -373,3 +373,4 @@
# Autodoc config
autoclass_content = 'both'
autodoc_member_order = 'bysource'
autodoc_mock_imports = ['grpc']

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pylintrc
# DEFAULT: variable-rgx=[a-z_][a-z0-9_]{2,30}$
# RATIONALE: Prefer longer, more specific variable names to avoid ambiguity.
# especially for methods that implement wordy rfcs or use wordy libraries.
variable-rgx=[a-z_][a-z0-9_]{2,40}$

This comment was marked as spam.

Sign in to view

@nathanielmanistaatgoogle
Copy link

No idea why that might be happening.

@theacodes
Copy link
Contributor Author

No idea why that might be happening.

Does gRPC support PyPy? Surely it must support 2.7, but it's failing there too.

@nathanielmanistaatgoogle
Copy link

PyPy support isn't completely formalized, but if it's not working for you we definitely want to know. You shouldn't have to disable gRPC Python when running with PyPy.

@theacodes
Copy link
Contributor Author

PyPy support isn't completely formalized, but if it's not working for you we definitely want to know. You shouldn't have to disable gRPC Python when running with PyPy.

Do you have a tracking bug on gRPC for this?

@theacodes
Copy link
Contributor Author

@bjwatson @geigerj seems I've satisfied @dhermes and @nathanielmanistaatgoogle, can you please take a look?

@nathanielmanistaatgoogle
Copy link

@jonparrott: issue 4221 tracks formal PyPy support; if you've found something not working I'm sure @thunderboltsid will want to hear about it.

nathanielmanistaatgoogle
nathanielmanistaatgoogle suggested changes Nov 1, 2016
View reviewed changes
tests/transport/test_grpc.py

import pytest

try:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@theacodes
Copy link
Contributor Author

@nathanielmanistaatgoogle it just seems to not compile on Travis. I'll link over to the build on that issue.

@nathanielmanistaatgoogle
Copy link

If I'm reading the Travis output correctly, this passes on 2.7 now, right? Did you find any reason why it was failing yesterday? PyPy is now the only known problematic environment?

@theacodes
Copy link
Contributor Author

theacodes commented Nov 1, 2016 •
edited
Loading

If I'm reading the Travis output correctly, this passes on 2.7 now, right?

Correct.

Did you find any reason why it was failing yesterday?

Not sure, but I'm guessing it was the addition of absolute_import that fixed it.

PyPy is now the only known problematic environment?

Correct.

@geigerj geigerj mentioned this pull request Nov 2, 2016
Merged
@theacodes theacodes merged commit b9897dc into master Nov 3, 2016
@theacodes theacodes deleted the grpc-transport branch November 3, 2016 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes approved these changes

@nathanielmanistaatgoogle nathanielmanistaatgoogle nathanielmanistaatgoogle approved these changes

@soltanmm-google soltanmm-google soltanmm-google left review comments

@geigerj geigerj geigerj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@theacodes @nathanielmanistaatgoogle @dhermes @geigerj @soltanmm-google @bjwatson