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

Generate Logging #97

Merged
merged 9 commits into from
Aug 29, 2016
Merged

Conversation

michaelbausor
Copy link
Contributor

This PR separates the generation of Logging from Pubsub in #62

One point for discussion/review is that the json files with retry settings are in a top level directory called resources. What would be the best way to provide these files?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 10, 2016
@@ -8,6 +8,9 @@
<filter>
<whitelist>
<directory suffix=".php">src</directory>
<exclude>
<directory suffix=".php">src/Logging/V2</directory>

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@garrettjonesgoogle
Copy link
Member

Is there anything else that needs to be addressed on this PR before it can be merged?

@dwsupplee
Copy link
Contributor

This is looking good. The only issue on this PR I would like to discuss still is in regards to @michaelbausor's point regarding the resources directory.

We haven't decided whether we will publish each API as it's own package yet. If we do, however, placing the resource files in src/Logging would help make the transition easier. What do you think?

@michaelbausor
Copy link
Contributor Author

Does it make sense to place them inside the major version folder? e.g. src/Logging/V2/resources That way it will be easier to manage API changes between major versions that cause the retry settings to change.

@dwsupplee
Copy link
Contributor

That sounds perfect!


// TODO load the client config in a more package-friendly way
// https://github.com/googleapis/toolkit/issues/332
$clientConfigJsonString = file_get_contents('./resources/config_service_v2_client_config.json');

This comment was marked as spam.

@bshaffer
Copy link
Contributor

Why do the classes end in V2API? The V2 namespace is enough to prevent collisions.

@michaelbausor
Copy link
Contributor Author

We are appending the Api portion - the other parts are coming from the logging proto, which is not following the style guide. So for example in pubsub they have named their interface Publisher, and we generate classes named PublisherApi. Instead of just calling their interfaces Logging, Metrics and Config, the interfaces are named LoggingServiceV2, MetricsServiceV2 and ConfigServiceV2, so we generate LoggingServiceV2Api, etc.

@michaelbausor
Copy link
Contributor Author

Sorry I have let this sit for a week. I have updated the logging surface with our latest generator. I also updated the google/auth dependency to 0.10, which is required by google/gax to use the caching functionality that has been added.

Please let me know if there are more issues that need to be addressed.

@dwsupplee
Copy link
Contributor

dwsupplee commented Aug 24, 2016

Thank you for the updates.

Should #62 lead this? It looks like it includes the necessary dependencies. On that note, though, what do you think about moving those dependencies from require to suggest since we can't rely on a user having the gRPC extension installed?

@michaelbausor
Copy link
Contributor Author

That seems sensible to me. @garrettjonesgoogle what are your thoughts?

This PR should probably not block on #62, because the PubSub surface does not yet support IAM methods. Support for those should be coming soon, but isn't available yet.

I will add the dependencies to this PR. Note also that since Stanley has flagged release versions for grpc (https://packagist.org/packages/grpc/grpc#v1.0.0) and protobuf (https://packagist.org/packages/stanley-cheung/protobuf-php#v0.6) we should not require "minimum-stability": "dev"

@garrettjonesgoogle
Copy link
Member

@michaelbausor @dwsupplee Moving the dependencies from require to suggest sounds like a good idea.

@dwsupplee dwsupplee merged commit 9173aef into googleapis:master Aug 29, 2016
@michaelbausor michaelbausor deleted the generate-logging branch August 29, 2016 15:46
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.

None yet

6 participants