Draft: Selective api poc - #3129
Conversation
Sorry, something went wrong.
…atches proto package in parser. other cleanups and minor fixes.
| serviceDescriptor -> { | ||
| List<MethodDescriptor> methodsList = serviceDescriptor.getMethods(); | ||
| if (methodsList.isEmpty()) { | ||
| List<MethodDescriptor> methodListSelected = |
There was a problem hiding this comment.
This scenario is in case that all methods from a service are excluded?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. And this is likely now e.g. in vertexai's requirement it only exposes methods from 2 services.
Sorry, something went wrong.
| } | ||
|
|
||
| private static Optional<List<String>> getInclusionMethodListFromServiceYaml( | ||
| Optional<com.google.api.Service> serviceYamlProtoOpt, String protoPackage) { |
There was a problem hiding this comment.
It is usually preferred to not passing around Optional if we can check it before calling the method.
Sorry, something went wrong.
There was a problem hiding this comment.
I want to follow-up on this from our discussion earlier and get @blakeli0 your opinion on this. (FYI, I've combined this method into shouldIncludeMethodInGeneration() )
For now, I am slightly inclined to keep this Optional<com.google.api.Service> serviceYamlProtoOpt as input for 2 reasons:
- The pattern of passing in this Optional is repeated in this Parser class, a larger scale refactor is needed to remove these.
- Because this service yaml is an optional config file that only some libraries have, it sort of make sense to have this Optional representation. And I find the current way can better encapsulate this piece of logic: shouldIncludeMethodInGeneration() has all the information to decide if this method should or not be included. I could check serviceYamlProtoOpt before calling the method, but the 2 occurrences would repeat the logic in a reverse way, and it might be less readable for debugging in the future.
Does this makes sense to you? WDYT?
Sorry, something went wrong.
| } | ||
|
|
||
| @Test | ||
| void selectiveGenerationTest_shouldExcludeUnusedResourceNames() { |
There was a problem hiding this comment.
This test verifies that unused resource names (associated with methods not in inclusion list) will not be populated to GapicContext.
Sorry, something went wrong.
| Transport.GRPC); | ||
| } | ||
|
|
||
| private static boolean shouldIncludeMethodInGeneration( |
There was a problem hiding this comment.
Can we make this a default scope method so we can test it directly? This would enabled us testing more scenarios like the non matching packages, it would also simplify the golden tests.
Sorry, something went wrong.
There was a problem hiding this comment.
We probably don't need this yaml and selective_api_generation_no_publishing_v1beta1.yaml as the scenarios should be tested in ParserTest directly.
Sorry, something went wrong.
#3323) following changes in client.proto that propagated to `ClientProto.java` ([pr](https://github.com/googleapis/sdk-platform-java/pull/3309/files#diff-44c330ef5cfa380744be6c58a14aa543edceb6043d5b17da7b32bb728ef5d85f)), apply changes from poc pr (#3129). For context: [go/selective-api-gen-java-one-pager](http://goto.google.com/selective-api-gen-java-one-pager), b/356380016
|
implemented in #3323. closing |
Sorry, something went wrong.
#3323) following changes in client.proto that propagated to `ClientProto.java` ([pr](https://github.com/googleapis/sdk-platform-java/pull/3309/files#diff-44c330ef5cfa380744be6c58a14aa543edceb6043d5b17da7b32bb728ef5d85f)), apply changes from poc pr (googleapis/sdk-platform-java#3129). For context: [go/selective-api-gen-java-one-pager](http://goto.google.com/selective-api-gen-java-one-pager), b/356380016
DO NOT MERGE.
This is a draft pr as prototype to changes for selective api generation feature.
The changes in this pr include api-common-java changes copied over manually for testing purposes.