test: restore showcase autogen IT #2895
Conversation
diegomarquezp
changed the title
* chore(main): release 5.3.0 * chore(main): release 5.3.0 --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
| </manifest> | ||
| </archive> | ||
| <descriptorRefs> | ||
| <descriptorRef>jar-with-dependencies</descriptorRef> |
There was a problem hiding this comment.
This is in order to run the spring generator jar using dependencies such as protobuf classes. This was something bazel took care about with the previous setup.
| @@ -130,6 +130,7 @@ public EchoSettings echoSettings( | |||
| clientSettingsBuilder | |||
| .setCredentialsProvider(this.credentialsProvider) | |||
| .setTransportChannelProvider(defaultTransportChannelProvider) | |||
| .setEndpoint(EchoSettings.getDefaultEndpoint()) | |||
There was a problem hiding this comment.
Not sure if this is what we want when creating client settings. We can follow up with proper changes in gax.
cc: @lqiu96
There was a problem hiding this comment.
Do you have the error message if you don't add this line to set the endpoint? Ideally, we shouldn't have to set the endpoint ourselves but for this PR, I think it's fine we can't resolve the issue here.
Or if this isn't becuase of an error, is this because we're using an old version of the generator. Or perhaps this is just something spring generator does?
There was a problem hiding this comment.
Do you have the error message if you don't add this line to set the endpoint?
The error message:
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'echoClient' defined in com.google.showcase.v1beta1.spring.EchoSpringAutoConfiguration: Failed to instantiate [com.google.showcase.v1beta1.EchoClient]: Factory method 'echoClient' threw exception with message: Invalid host or port: .googleapis.com 443
at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:648)
at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:636)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1335)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1165)
It fails to resolve the endpoint if we don't explicitly set it: Invalid host or port: .googleapis.com 443
Or if this isn't because of an error, is this because we're using an old version of the generator. Or perhaps this is just something spring generator does?
I don't think this is very different from a normal use case of instantiating a client with a few settings.
Here we create the settings:
Here we create the client with these settings:
There was a problem hiding this comment.
It fails to resolve the endpoint if we don't explicitly set it: Invalid host or port: .googleapis.com 443
Ah, I think this is an edge case due to the the showcase protos. The showcase protos have a default_host value that won't match a "normal" google endpoint. For example, the echo proto's service_host value: https://github.com/googleapis/gapic-showcase/blob/4e7c30a5d5df8aac30e8b8f1e6f71809fa9d20f6/schema/google/showcase/v1beta1/echo.proto#L45
where as a normal endpoint would be something like cloudassets.googleapis.com.
The error above is complaining that we can't populate the serviceName part of the endpoint and that's because the showcase protos don't have one. That's why you'd have to explicitly call setEndpoint() yourself.
This looks like it's changing the code inside the generator, which I believe would impact all of spring code gen. The endpoint normally should be constructed internally for non-showcase services and I believe having the generator explicitly call setEndpoint() may be imcompatible with enabling certain features.
In the showcase ITs, we have this same problem. But we solve it by explicitly calling setEndpoint() in the handwritten layers: https://github.com/googleapis/sdk-platform-java/blob/5799827a864be627bac03969cc178efc9d6170aa/showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/util/TestClientInitializer.java#L67
I'm not too familiar with the showcase autogen ITs. Are there any handwritten parts that we can do something similar or are these the only tests that are generated + run?
There was a problem hiding this comment.
Are there any handwritten parts that we can do something similar or are these the only tests that are generated + run?
This is the handwritten test.Compare to GAPIC showcase tests, this test though does not create a client, instead, it fetches the client from existing Spring beans. So it's hard to override the endpoint in the tests since the whole point of SpringCodeGen is to create a client for customers.
I think this change might be unavoidable for now, and should not impact any existing features, because SpringCodeGen does not expose a property for customers to set endpoint or universe domain either. We can revisit this decision once we get to implement universe domain in spring-cloud-gcp. WDYT @lqiu96 ?
There was a problem hiding this comment.
I see. I'm not too familiar with spring so I don't know of a way to do this, but we are going to have to remove the explicit setting for the default endpoint at some point (i.e. whenever we enable universe domain). This will require a workaround for the showcase clients in the future or more configurations generated by the generator.
I think for now we can proceed with explicitly setting the default endpoint.
|
|
||
| # We will use the generation tools from library_generation | ||
| pushd library_generation/utils | ||
| source utilities.sh |
There was a problem hiding this comment.
How many utility methods in utilities.sh are we relying on? If it's not too much, I would prefer just duplicate the logic, because this creates dependencies of our hermetic build scripts, and we may accidentally break SpringCodeGen scripts.
There was a problem hiding this comment.
I'm moving them to generate-showcase-utilities
|
|
||
| proto_path="schema/google/showcase/v1beta1" | ||
| proto_files=$(find "${proto_path}" -type f -name "*.proto" | LC_COLLATE=C sort) | ||
| gapic_additional_protos="google/iam/v1/iam_policy.proto google/cloud/location/locations.proto" |
There was a problem hiding this comment.
I don't think we need these additional protos, same thing for rest_numeric_enums, transport, gapic_yaml, include_samples etc. Can you try to remove them and see if the checks are still passing?
There was a problem hiding this comment.
@blakeli0 when removing the additional protos, the generated code has missing iam methods. For example:
diff -r /usr/local/google/home/diegomarquezp/Desktop/spring-cloud-gcp/spring-cloud-generator/scripts/../showcase/showcase-spring-starter-generated/src/main/java/com/google/showcase/v1beta1/spring/SequenceServiceSpringAutoConfiguration.java /usr/local/google/home/diegomarquezp/Desktop/spring-cloud-gcp/spring-cloud-generator/scripts/../showcase/showcase-spring-starter/src/main/java/com/google/showcase/v1beta1/spring/SequenceServiceSpringAutoConfiguration.java
98a99,101
> if (this.clientProperties.getUseRest()) {
> return SequenceServiceSettings.defaultHttpJsonTransportProviderBuilder().build();
> }
123c126,134
< SequenceServiceSettings.Builder clientSettingsBuilder = SequenceServiceSettings.newBuilder();
---
> SequenceServiceSettings.Builder clientSettingsBuilder;
> if (this.clientProperties.getUseRest()) {
> clientSettingsBuilder = SequenceServiceSettings.newHttpJsonBuilder();
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Using REST (HTTP/JSON) transport.");
> }
> } else {
> clientSettingsBuilder = SequenceServiceSettings.newBuilder();
> }
186a198,224
> RetrySettings listLocationsRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.listLocationsSettings().getRetrySettings(), serviceRetry);
> clientSettingsBuilder.listLocationsSettings().setRetrySettings(listLocationsRetrySettings);
>
> RetrySettings getLocationRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.getLocationSettings().getRetrySettings(), serviceRetry);
> clientSettingsBuilder.getLocationSettings().setRetrySettings(getLocationRetrySettings);
>
> RetrySettings setIamPolicyRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.setIamPolicySettings().getRetrySettings(), serviceRetry);
> clientSettingsBuilder.setIamPolicySettings().setRetrySettings(setIamPolicyRetrySettings);
>
> RetrySettings getIamPolicyRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.getIamPolicySettings().getRetrySettings(), serviceRetry);
> clientSettingsBuilder.getIamPolicySettings().setRetrySettings(getIamPolicyRetrySettings);
>
> RetrySettings testIamPermissionsRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.testIamPermissionsSettings().getRetrySettings(), serviceRetry);
> clientSettingsBuilder
> .testIamPermissionsSettings()
> .setRetrySettings(testIamPermissionsRetrySettings);
>
254a293,346
> }
> }
> Retry listLocationsRetry = clientProperties.getListLocationsRetry();
> if (listLocationsRetry != null) {
> RetrySettings listLocationsRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.listLocationsSettings().getRetrySettings(), listLocationsRetry);
> clientSettingsBuilder.listLocationsSettings().setRetrySettings(listLocationsRetrySettings);
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Configured method-level retry settings for listLocations from properties.");
> }
> }
> Retry getLocationRetry = clientProperties.getGetLocationRetry();
> if (getLocationRetry != null) {
> RetrySettings getLocationRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.getLocationSettings().getRetrySettings(), getLocationRetry);
> clientSettingsBuilder.getLocationSettings().setRetrySettings(getLocationRetrySettings);
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Configured method-level retry settings for getLocation from properties.");
> }
> }
> Retry setIamPolicyRetry = clientProperties.getSetIamPolicyRetry();
> if (setIamPolicyRetry != null) {
> RetrySettings setIamPolicyRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.setIamPolicySettings().getRetrySettings(), setIamPolicyRetry);
> clientSettingsBuilder.setIamPolicySettings().setRetrySettings(setIamPolicyRetrySettings);
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Configured method-level retry settings for setIamPolicy from properties.");
> }
> }
> Retry getIamPolicyRetry = clientProperties.getGetIamPolicyRetry();
> if (getIamPolicyRetry != null) {
> RetrySettings getIamPolicyRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.getIamPolicySettings().getRetrySettings(), getIamPolicyRetry);
> clientSettingsBuilder.getIamPolicySettings().setRetrySettings(getIamPolicyRetrySettings);
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Configured method-level retry settings for getIamPolicy from properties.");
> }
> }
> Retry testIamPermissionsRetry = clientProperties.getTestIamPermissionsRetry();
> if (testIamPermissionsRetry != null) {
> RetrySettings testIamPermissionsRetrySettings =
> RetryUtil.updateRetrySettings(
> clientSettingsBuilder.testIamPermissionsSettings().getRetrySettings(),
> testIamPermissionsRetry);
> clientSettingsBuilder
> .testIamPermissionsSettings()
> .setRetrySettings(testIamPermissionsRetrySettings);
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace(
> "Configured method-level retry settings for testIamPermissions from properties.");
There was a problem hiding this comment.
Removing rest_numeric_enums causes issues too:
diff -r /usr/local/google/home/diegomarquezp/Desktop/spring-cloud-gcp/spring-cloud-generator/scripts/../showcase/showcase-spring-starter-generated/src/main/java/com/google/showcase/v1beta1/spring/TestingSpringAutoConfiguration.java /usr/local/google/home/diegomarquezp/Desktop/spring-cloud-gcp/spring-cloud-generator/scripts/../showcase/showcase-spring-starter/src/main/java/com/google/showcase/v1beta1/spring/TestingSpringAutoConfiguration.java
96a97,99
> if (this.clientProperties.getUseRest()) {
> return TestingSettings.defaultHttpJsonTransportProviderBuilder().build();
> }
118c121,129
< TestingSettings.Builder clientSettingsBuilder = TestingSettings.newBuilder();
---
> TestingSettings.Builder clientSettingsBuilder;
> if (this.clientProperties.getUseRest()) {
> clientSettingsBuilder = TestingSettings.newHttpJsonBuilder();
> if (LOGGER.isTraceEnabled()) {
> LOGGER.trace("Using REST (HTTP/JSON) transport.");
> }
> } else {
> clientSettingsBuilder = TestingSettings.newBuilder();
> }
I'll keep checking on the other params
There was a problem hiding this comment.
include_samples and gapic_yaml were unnecessary. The other ones produce changes if removed.
There was a problem hiding this comment.
SG, we can keep the ones produce changes.
| GAPIC_SHOWCASE_CLIENT_VERSION=$(mvn help:evaluate -Dexpression=project.version -q -DforceStdout) | ||
| pushd showcase | ||
| # For local development, we cleanup any traces of previous runs | ||
| rm -rdf output |
There was a problem hiding this comment.
Everything below uses ${output_folder}, but this uses literal value output.
|
LGTM |
|
Fixes #2214 🔨
Context
So far, this test used bazel + a few string manipulations in sdk-platform-java's showcase BUILD file and WORKSPACE file. This wasn't working since we migrated showcase generation to our hermetic build scripts. This PR transforms the test into using protoc directly
Approach
It's very similar to
generate_library.shin the sense that we infer the protoc version and then use the spring-cloud-generator jar as a plugin to protoc to compile the showcase protos, which are downloaded in the script.Other changes
endpointto[Service]Settings.getDefaultEndpoint(). The beans would fail to load otherwise. cc: @lqiu96jar-with-dependenciesfor spring-cloud-generator. Apparently bazel was able to figure out how to inject dependencies in the generation runtime. Now we need to create a jar with all dependencies as we run protoc directly with the jar.protoctakes plugins.