fix: change to generation script in manipulating googleapis/WORKSPACE. #3120
Conversation
| @@ -91,7 +91,7 @@ function modify_workspace_file(){ | |||
| # delete existing maven_install rules | |||
| buildozer 'delete' ${path_to_workspace}:%maven_install | |||
| # add custom maven_install rules | |||
| perl -pi -e "s{(^_gapic_generator_java_version[^\n]*)}{\$1\n$(cat ${path_to_modification_string})}" ${path_to_workspace} | |||
| perl -pi -e "s{(^api_dependencies()[^\n]*)}{\$1\n$(cat ${path_to_modification_string})}" ${path_to_workspace} | |||
There was a problem hiding this comment.
qq, is this adding the bazel rule to the lines after api_dependencies()?
|
I think the changes make sense and LGTM. I think @blakeli0 should give the final approval since he may have more context regarding the original WORKSPACE changes. |
|
Build error was fixed. but observing showcase unit test failures. More investigation needed, but at first glance, this error message referencing time.Duration looks suspicious, and may need spring generator changes to fix. Update: it is the testing mocks that's affected. Will add a fix soon. |
| @@ -196,8 +196,8 @@ void testCustomTransportChannelProviderUsedWhenProvided() throws IOException { | |||
| // TransportChannelProvider bean | |||
| when(mockApiCallContext.withCredentials(any())).thenReturn(mockApiCallContext); | |||
| when(mockApiCallContext.withTransportChannel(any())).thenReturn(mockApiCallContext); | |||
| when(mockApiCallContext.withStreamWaitTimeout(any())).thenReturn(mockApiCallContext); | |||
There was a problem hiding this comment.
Is there going to be errors if we don't mock the behaviors of ApiCallContext? I guess somewhere in the client initialization logic these lines are called?
There was a problem hiding this comment.
Yes, removing will cause errors (the same ones we observe in the ci before this fix).
However, for the purpose of this test, instead of creating the mockTransportChannelProvider, I think it is also okay to create a real channelProvider and test it is actually used. The benefit is we free ourselves from implementation detail changes in the future.
E.g.
InstantiatingGrpcChannelProvider channelProvider = InstantiatingGrpcChannelProvider
.newBuilder()
.setEndpoint("localhost:7469")
.setMaxInboundMessageSize(Integer.MAX_VALUE)
.build();
…Provider instead of using mocks.
|
|
||
| InstantiatingGrpcChannelProvider channelProvider = | ||
| InstantiatingGrpcChannelProvider.newBuilder() | ||
| .setEndpoint("localhost:7469") |
There was a problem hiding this comment.
Are these two fields required?
There was a problem hiding this comment.
nope. I can remove that
| InstantiatingGrpcChannelProvider.newBuilder() | ||
| .setEndpoint("localhost:7469") | ||
| .setMaxInboundMessageSize(Integer.MAX_VALUE) | ||
| .build(); | ||
| contextRunner |
There was a problem hiding this comment.
To @lqiu96 's concerns about resource closing from offline discussion:
Yes, createChannel() is actually called in this test. But Spring's ApplicationContextRunner (contextRunner in this test) is actually responsible of managing and closing all beans and data sources it manages.
There was a problem hiding this comment.
Ok, gotcha. Thanks for checking!
|
Need this adjustment due to changes in
googleapis/WORKSPACEchange here.To fix:
Spring Auto Generation is failing: https://github.com/GoogleCloudPlatform/spring-cloud-gcp/actions/runs/10376314314
Root cause is due to location change of
maven_installtarget in WORKSPACE, the substitution is done withsedcommand targeting after a previous line. This is not the right place to have thismaven_installtarget anymore because it needs to come after these lines.Generation process tested successfully in local.
Also added in this pr: