Resource names across different protofiles #2711

andreamlin · 2019-04-11T19:18:59Z

GAPIC config v1 has a way of limiting use of a resource entity to a particular interface, i.e. service in protobuf. API config spec can define a resource name in a protofile, though it cannot specify a particular service for the resource name.

For all services in a protofile, generate the resource name artifacts that had resources defined in that protofile.

This is so APIs like Vision, which is composed of multiple protofiles, of which only some protos files have defined resource names, to only generate resource name utils in the classes where they are used. See the updated ruby baseline for an example.

codecov · 2019-04-11T19:19:21Z

Codecov Report

Merging #2711 into master will decrease coverage by 0.08%.
The diff coverage is 68.1%.

@@             Coverage Diff              @@
##             master    #2711      +/-   ##
============================================
- Coverage     86.91%   86.83%   -0.09%     
+ Complexity     5623     5621       -2     
============================================
  Files           468      469       +1     
  Lines         22423    22467      +44     
  Branches       2440     2440              
============================================
+ Hits          19490    19509      +19     
- Misses         2082     2096      +14     
- Partials        851      862      +11

Impacted Files	Coverage Δ	Complexity Δ
...google/api/codegen/config/PageStreamingConfig.java	`63.21% <ø> (ø)`	`16 <0> (ø)`	⬇️
...le/api/codegen/config/ResourceNameOneofConfig.java	`48.57% <38.46%> (-2.2%)`	`10 <3> (ø)`
...m/google/api/codegen/config/LongRunningConfig.java	`55.55% <68.42%> (+7.13%)`	`14 <7> (+3)`	⬆️
.../google/api/codegen/config/GapicProductConfig.java	`79.04% <90%> (-0.08%)`	`92 <0> (ø)`
...oogle/api/codegen/util/ConfigVersionValidator.java	`91.3% <91.3%> (ø)`	`9 <9> (?)`
...pi/codegen/viewmodel/StaticLangClientFileView.java	`80% <0%> (-20%)`	`4% <0%> (-1%)`
...former/csharp/CSharpGapicSmokeTestTransformer.java	`74.68% <0%> (-6.33%)`	`8% <0%> (-3%)`
...nsformer/csharp/CSharpBasicPackageTransformer.java	`88.57% <0%> (-5.72%)`	`12% <0%> (-2%)`
...java/com/google/api/codegen/util/LanguageUtil.java	`21.05% <0%> (-5.27%)`	`2% <0%> (-1%)`
...n/transformer/java/JavaModelTypeNameConverter.java	`86.01% <0%> (-4.2%)`	`38% <0%> (-3%)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a77c4...ea6ea8d. Read the comment docs.

codecov · 2019-04-11T19:19:21Z

Codecov Report

❗ No coverage uploaded for pull request base (gapic_config_v2@6a72227). Click here to learn what that means.
The diff coverage is 88.46%.

@@                Coverage Diff                 @@
##             gapic_config_v2    #2711   +/-   ##
==================================================
  Coverage                   ?   86.94%           
  Complexity                 ?     5643           
==================================================
  Files                      ?      469           
  Lines                      ?    22474           
  Branches                   ?     2442           
==================================================
  Hits                       ?    19540           
  Misses                     ?     2083           
  Partials                   ?      851

Impacted Files	Coverage Δ	Complexity Δ
...e/api/codegen/config/SingleResourceNameConfig.java	`81.81% <ø> (ø)`	`9 <0> (?)`
...le/api/codegen/config/FixedResourceNameConfig.java	`50% <ø> (ø)`	`6 <0> (?)`
...oogle/api/codegen/config/GapicInterfaceConfig.java	`80.48% <76.47%> (ø)`	`47 <4> (?)`
.../google/api/codegen/config/GapicProductConfig.java	`79.92% <94.28%> (ø)`	`95 <7> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a72227...ea6ea8d. Read the comment docs.

andreamlin · 2019-04-11T19:25:21Z

.../java/com/google/api/codegen/protoannotations/testdata/ruby_library_no_gapic_config.baseline

+      # @param return_ [String]
+      # @return [String]
+      def self.return_path shelf, book, return_
+        RETURN_PATH_TEMPLATE.render(


This RETURN_PATH_TEMPLATE comes from the pre-existing resource entity defined in another_service.proto, and now it shows up in the MyProtoClient generated class!

Note that there were no changes to the proto test files, only the gapic-generator sources.

andreamlin · 2019-04-11T19:26:23Z

.../java/com/google/api/codegen/protoannotations/testdata/ruby_library_no_gapic_config.baseline

-
-      DELETED_BOOK_PATH_TEMPLATE = Google::Gax::PathTemplate.new(
-        "_deleted-book_"
+      ARCHIVED_BOOK_PATH_TEMPLATE = Google::Gax::PathTemplate.new(


This is just re-ordering of the resource name artifacts. i thought it would make more sense to order them alphabetically, as it's deterministic and predictable.

How were they ordered previously? Is this going to cause churn when we switch a client from v1 to v2 config?

Pretty sure it was ordered by the order in GAPIC interface's collections, followed by any top-level non-duplicate API collections

So if there's no collections field in the GAPIC config v2 to order the resource names, yeah we'll most likely get churn

OK. Not sure whether there is a coherent alternative, but I think that avoiding churn is a worthwhile goal. If it's not possible, we may just need to do a regeneration ahead of time to get the churn out of the way.

I think we would have to do the regeneration pre-party, yeah. We could either draw ire now by changing the current generator to order resource name artifacts alphabetically (a billion changes at once for Yoshi), or API-by-API inside GAPIC configs (manual work on our part, but diffs are spread over time for Yoshi).

andreamlin · 2019-04-11T19:42:27Z

PTAL

andreamlin · 2019-04-11T20:56:52Z

@michaelbausor any more comments?

* Add Gapic config v2 (#2665) * Whittling down config_v2 (#2666) * Add ConfigV2 Validator (#2672) * AutoValue LongRunningConfig; always use gapic config's polling settings (#2698) * ResourceNameOneofConfig fixes (#2704) * Start parsing GAPIC config v2 (#2703) * Bring back timeout millis in GAPIC config v2 (#2708) * Resource names across different protofiles (#2711) * Fix missing default retries (#2718) * Bug fixes for gapic config v2 parsing (#2717)

* Add Gapic config v2 (googleapis#2665) * Whittling down config_v2 (googleapis#2666) * Add ConfigV2 Validator (googleapis#2672) * AutoValue LongRunningConfig; always use gapic config's polling settings (googleapis#2698) * ResourceNameOneofConfig fixes (googleapis#2704) * Start parsing GAPIC config v2 (googleapis#2703) * Bring back timeout millis in GAPIC config v2 (googleapis#2708) * Resource names across different protofiles (googleapis#2711) * Fix missing default retries (googleapis#2718) * Bug fixes for gapic config v2 parsing (googleapis#2717)

andreamlin added 6 commits April 10, 2019 14:24

multiple services

3b04b2a

more test source changes

b66c82d

Merge branch 'gapic_config_v2' into multiple_services

5f9f9d1

merge in gapic_config_v2

af84354

rm interfacename

11e0003

rm more interfacename

ea6ea8d

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 11, 2019

andreamlin changed the base branch from master to gapic_config_v2 April 11, 2019 19:19

andreamlin commented Apr 11, 2019

View reviewed changes

andreamlin requested a review from michaelbausor April 11, 2019 19:26

andreamlin changed the title ~~Resource names across multiple services~~ Resource names across different protofiles Apr 11, 2019

michaelbausor approved these changes Apr 11, 2019

View reviewed changes

andreamlin merged commit a2e9642 into googleapis:gapic_config_v2 Apr 11, 2019

andreamlin deleted the multiple_services branch April 11, 2019 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource names across different protofiles #2711

Resource names across different protofiles #2711

andreamlin commented Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

andreamlin Apr 11, 2019

andreamlin Apr 11, 2019 •

edited

Loading

michaelbausor Apr 11, 2019

andreamlin Apr 11, 2019

michaelbausor Apr 11, 2019

andreamlin Apr 11, 2019

andreamlin commented Apr 11, 2019

andreamlin commented Apr 11, 2019

Resource names across different protofiles #2711

Resource names across different protofiles #2711

Conversation

andreamlin commented Apr 11, 2019 • edited Loading

codecov bot commented Apr 11, 2019 • edited Loading

Codecov Report

codecov bot commented Apr 11, 2019 • edited Loading

Codecov Report

andreamlin Apr 11, 2019

Choose a reason for hiding this comment

andreamlin Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

michaelbausor Apr 11, 2019

Choose a reason for hiding this comment

andreamlin Apr 11, 2019

Choose a reason for hiding this comment

michaelbausor Apr 11, 2019

Choose a reason for hiding this comment

andreamlin Apr 11, 2019

Choose a reason for hiding this comment

andreamlin commented Apr 11, 2019

andreamlin commented Apr 11, 2019

andreamlin commented Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading

andreamlin Apr 11, 2019 •

edited

Loading