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

Pub/Sub SPI: Adding IAM methods and auto-generated unit tests #1219

Closed

Conversation

garrettjonesgoogle
Copy link
Member

Note: grpc-google-pubsub-v1 depends on a new package grpc-google-iam-v1, which contains IAMPolicy and Policy.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 31, 2016
@mziccard
Copy link
Contributor

A couple of questions on dependencies:

  • grpc-google-iam-v1 contains iam classes that will be used by all services, right?
  • Why does grpc-google-common-protos depend on protobuf-java 3.0.0-beta-3 instead of the latest protobuf-java 3.0.0?

@garrettjonesgoogle
Copy link
Member Author

  • yes, grpc-google-iam-v1 will be shared by multiple services.
  • I haven't updated the protobuf-java dependency in order to isolate changes. I will do a batch upgrade of grpc to 1.0.0 and protobuf to 3.0.0 in one go, without other changes at the same time.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.434% when pulling 96f22d3 on garrettjonesgoogle:master into 7cd60df on GoogleCloudPlatform:master.

@mziccard
Copy link
Contributor

mziccard commented Sep 1, 2016

@garrettjonesgoogle another general question: is there a real need for having a copy of setIamPolicy, setIamPolicy and testIamPermissions in both PublisherApi and SubscriberApi? It seems like a lot of duplicated code to me. Will other services use/require this approach as well? Did you consider putting the three methods in a dedicated PubSubIamApi class?

@garrettjonesgoogle
Copy link
Member Author

It's the general pattern for IAM. In services that follow the suggested pattern (see https://github.com/googleapis/googleapis/blob/master/google/genomics/v1/datasets.proto ), the methods are redeclared in each service declaration. This allows their documentation and http paths to be customized. Pub/Sub doesn't follow the suggested pattern in the proto declarations, although it shows up the correct way in the http view (see https://developers.google.com/apis-explorer/#p/pubsub/v1/ ).

@mziccard
Copy link
Contributor

mziccard commented Sep 1, 2016

Will setting the policy for a topic resource via the SubscriberApi (or for a subscription via the PublisherApi) fail? Or is this just for having specific docs and http endpoints?

@garrettjonesgoogle
Copy link
Member Author

The way it's set up currently, it will fail validation when the flattened methods are used, but the request-based methods would not fail. The http endpoints actually have zero impact on the gRPC call; we just use the http endpoint information to inform our codegen (we derive the validation from it).

@mziccard
Copy link
Contributor

mziccard commented Sep 7, 2016

I am closing this, rebased on top of master in #1229

@mziccard mziccard closed this Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants