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

Regenerate vision client #4161

Closed
wants to merge 1 commit into from
Closed

Regenerate vision client #4161

wants to merge 1 commit into from

Conversation

yoshi-automation
Copy link
Contributor

This PR was generated using Autosynth. 🌈

Here's the log from Synthtool:

synthtool > You are running the synthesis script directly, this will be disabled in a future release of Synthtool. Please use python3 -m synthtool instead.
synthtool > Ensuring dependencies.
synthtool > Pulling artman image.
latest: Pulling from googleapis/artman
Digest: sha256:2f6b261ee7fe1aedf238991c93a20b3820de37a343d0cacf3e3e9555c2aaf2ea
Status: Image is up to date for googleapis/artman:latest
synthtool > Cloning googleapis.
synthtool > Running generator for google/cloud/vision/artman_vision_v1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/java.
synthtool > Running java formatter on 62 files
synthtool > Running java formatter on 2 files
synthtool > Running java formatter on 162 files
synthtool > Running generator for google/cloud/vision/artman_vision_v1p1beta1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/java.
synthtool > Running java formatter on 62 files
synthtool > Running java formatter on 1 files
synthtool > Running java formatter on 67 files
synthtool > Running generator for google/cloud/vision/artman_vision_v1p2beta1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/java.
synthtool > Running java formatter on 62 files
synthtool > Running java formatter on 1 files
synthtool > Running java formatter on 91 files
synthtool > Running generator for google/cloud/vision/artman_vision_v1p3beta1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/java.
synthtool > Running java formatter on 62 files
synthtool > Running java formatter on 2 files
synthtool > Running java formatter on 165 files
synthtool > Cleaned up 0 temporary directories.

@yoshi-automation yoshi-automation requested a review from a team as a code owner November 30, 2018 20:17
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 30, 2018
@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 4, 2018
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 4, 2018
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of breaking changes here.

"projects/{project}/locations/{location}/products/{product}/referenceImages/{image}");

/** Formats a string containing the fully-qualified path to represent a location resource. */
public static final String formatLocationName(String project, String location) {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@chingor13 chingor13 reopened this Dec 7, 2018
@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 7, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 7, 2019
@sduskis
Copy link
Contributor

sduskis commented Jan 16, 2019

@andreamlin, do you have any comments on this issue?

@andreamlin
Copy link
Contributor

Ok, found the reason for the removal of those functions: these formatResourceName functions are generated when the resource_name_generation section of the GAPIC config is empty (gapic-generator implementation). Because vision_gapic.yaml did not have a resource_name_generation section at the last refresh of the vision client, the resulting client library DID contain these formatting functions. Between the last refresh and this one, vision_gapic.yaml was populated with a resource_name_generation section.

One possible solution is to always print out these formatting functions; however this adds a lot of cruft to every other Java API library.

Another possible solution is to mark these formatting functions as @BetaApi after the fact and mark them for turn down. We would expect that eventually, per API, all resource messages are configured and these formatting functions wouldn't be necessary.

cc'ing @michaelbausor. This behavior was introduced years ago, in googleapis/gapic-generator#824.

@michaelbausor
Copy link
Contributor

Hmm. Another option - we add an optional configuration into the resource_name_generation, and update the generator to check for it. So the methods will be generated either when the resource_name_generation section is missing, or when it is present with the config set. This will allow an API that doesn't have a resource_name_generation config to specify one in a non-breaking way. But, it adds configuration, which is not desirable.

Separate question - how do we expect the generator to behave when we move to the proto annotation config spec?

@andreamlin
Copy link
Contributor

andreamlin commented Jan 18, 2019

@michaelbausor
That seems like a workable solution. It is only a boolean flag, and only used for instances like this.

re: proto annotation spec; for new API client libs, we would never enableStringFormatFunctions unless there were no Resource Name configs, in which case it's an irrelevant point because there will be no resource name entities to format. However, if we remove the GAPIC config and move to proto annotations for existing APIs like Vision that still have the String format functions, then they will broken just like in this PR. In that case, we can introduce a command-line option to enableStringFormatFunctions that will purely be used for maintaining backward compatibility (or maybe keep a config param exposed in GAPIC config).

@andreamlin
Copy link
Contributor

Opened gapic-generator issue to track this: googleapis/gapic-generator#2521.

@andreamlin
Copy link
Contributor

@michaelbausor @chingor13 Would it be possible to deprecate these functions (if/when a major version comes around)? I'm not sure what the use of them would be given that they are are only present when there are no messages that reference them.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
@sduskis sduskis added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Feb 11, 2019
@andreamlin andreamlin removed the status: blocked Resolving the issue is dependent on other work. label Feb 13, 2019
@andreamlin
Copy link
Contributor

This should be fixed now, if synthtool is run again. My local regen: andreamlin#2

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 13, 2019
@sduskis sduskis closed this Feb 14, 2019
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. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants