Feature/862 add spanner module #865
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Hi Twan, thank you for submitting the pull request for the Testcontainers Spanner module. I appreciate the effort you've put into it so far.
However, I have some suggestions that I believe could help improve the module implementation. Specifically, I noticed that there are some parts of the implementation that are not aligned with the other modules.
Additionally, I am wondering if the code that creates instances, databases, etc. (in StartAsync(CancellationToken)) is necessary for a successful container start (running the Spanner service). It seems like we are including tasks that should be the responsibility of the Google.Cloud.Spanner.* NuGet dependencies. This relates to #865 (comment).
Could you please provide some clarification on whether this code is necessary, or if it can be removed to make the module implementation more streamlined and consistent with other Testcontainers modules?
| { | ||
| await base.StartAsync(ct); | ||
|
|
||
| Environment.SetEnvironmentVariable(EnvironmentVariableEmulatorHost, $"{Hostname}:{GrpcPort}"); | ||
| #pragma warning disable S5332 | ||
| // warning S5332: Using http protocol is insecure. Use https instead. | ||
| // This doesn't apply here, as it is a testing setup only localhost connection encryption is not required. | ||
| _webClient.BaseAddress = new Uri($"http://{Hostname}:{RestPort}"); | ||
| #pragma warning restore S5332 | ||
|
|
||
| await CreateSpannerInstance(ct); | ||
| await CreateDatabase(ct); | ||
| } | ||
|
|
||
| private async Task CreateSpannerInstance(CancellationToken ct = default) | ||
| { | ||
| var resource = new | ||
| { | ||
| instanceId = _configuration.InstanceId, | ||
| instance = new | ||
| { | ||
| config = "emulator-config", | ||
| displayName = _configuration.InstanceId, | ||
| } | ||
| }; | ||
| string requestUri = $"v1/projects/{_configuration.ProjectId}/instances"; | ||
| string resourceDescription = "instance"; | ||
|
|
||
| await CreateResource(requestUri, resource, resourceDescription, ct); | ||
| } | ||
|
|
||
| private async Task CreateDatabase(CancellationToken ct = default) | ||
| { | ||
| var resource = new | ||
| { | ||
| createStatement = $"CREATE DATABASE `{_configuration.DatabaseId}`", | ||
| }; | ||
|
|
||
|
|
||
| string requestUri = $"v1/projects/{_configuration.ProjectId}/instances/{_configuration.InstanceId}/databases"; | ||
| string resourceDescription = "database"; | ||
|
|
||
| await CreateResource(requestUri, resource, resourceDescription, ct); | ||
| } | ||
|
|
||
| public async Task ExecuteDdlAsync(params string[] ddl) | ||
| { | ||
| string requestUri = $"v1/projects/{_configuration.ProjectId}/instances/{_configuration.InstanceId}/databases/{_configuration.DatabaseId}/ddl"; | ||
| var request = new { statements = ddl }; | ||
|
|
||
| var response = await _webClient.PatchAsJsonAsync(requestUri, request) | ||
| .ValidateAsync("update ddl"); | ||
|
|
||
| var operation = await response.Content.ReadFromJsonAsync<SpannerOperationDto>() ?? | ||
| throw new InvalidOperationException( | ||
| "Executing ddl resulted in a response with a body deserializing as SpannerOperation to null"); | ||
|
|
||
| await AwaitOperationAsync(operation); | ||
|
|
||
| if (operation.Error != null) | ||
| { | ||
| throw new InvalidOperationException($"Failed to execute ddl operation with error {operation.Error}"); | ||
| } | ||
| } | ||
|
|
||
| private async Task AwaitOperationAsync(SpannerOperationDto operation) | ||
| { | ||
| int delayInSeconds = 1; | ||
| const int maxDelayInSeconds = 10; | ||
|
|
||
| while (!operation.Done) | ||
| { | ||
| await Task.Delay(TimeSpan.FromSeconds(delayInSeconds)); | ||
| string operationName = operation.Name; | ||
| var operationResponse = await _webClient.GetAsync($"v1/{operationName}").ValidateAsync("get operation"); | ||
|
|
||
| operation = await operationResponse.Content.ReadFromJsonAsync<SpannerOperationDto>() ?? | ||
| throw new InvalidOperationException( | ||
| $"Getting operation resulted in null deserialization of body for operation name {operationName}"); | ||
| if (delayInSeconds * 2 < maxDelayInSeconds) | ||
| { | ||
| delayInSeconds *= 2; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| private async Task CreateResource(string requestUri, object resource, string resourceDescription, CancellationToken ct = default) | ||
| { | ||
| await _webClient.PostAsJsonAsync(requestUri, resource, cancellationToken: ct) | ||
| .ValidateAsync($"create {resourceDescription}", ct: ct); | ||
| } | ||
|
|
||
| public void Dispose() => _webClient.Dispose(); | ||
|
|
||
| protected override async ValueTask DisposeAsyncCore() | ||
| { | ||
| _webClient.Dispose(); | ||
| await base.DisposeAsyncCore(); | ||
| } |
There was a problem hiding this comment.
Are these configurations at all necessary to start a Spanner container? Usually, we do not include client tasks into Testcontainers. Developers should use the corresponding client dependency. It looks like Spanner has a dedicated dependency to create an instance or database?
The Google.Cloud.Spanner.Admin.Instance.V1 package should be used for Cloud Spanner instance administration, such as creating or deleting instances.
If necessary at all, can we move the container configuration to WithStartupCallback, e.g. like Couchbase? We try to configure the entire container until it is ready in the builder.
There was a problem hiding this comment.
It was inspired by what I saw in the postgresql, where you can include a script for initial setup. In our use cases it made sense to always have this available, but for m ore general purpose i can see it malkes sense to also have the emulator running without this, so I'll remove it from startup.
There was a problem hiding this comment.
It was inspired by what I saw in the postgresql, where you can include a script for initial setup.
But this is part (supported) by the PostgreSQL image, right? As previously mentioned, while the intention is nice, we try to avoid incorporating client-side tasks into modules. This is because it does not make sense for us to maintain vendor-specific functionalities when there is already a suitable client that can handle them. Instead, we strive to offer best practice configurations for common test use cases.
| { | ||
| await Task.Delay(TimeSpan.FromSeconds(delayInSeconds)); | ||
| string operationName = operation.Name; | ||
| var operationResponse = await _webClient.GetAsync($"v1/{operationName}").ValidateAsync("get operation"); |
There was a problem hiding this comment.
You can use the HttpWaitStrategy to wait for a specific HTTP response.
There was a problem hiding this comment.
It is now also used by the execute ddl, but if you prefer not to have anything in here that can also be done with a client library, we should remove that part i think.
| @@ -0,0 +1 @@ | |||
| [assembly: CollectionBehavior(DisableTestParallelization = true)] | |||
There was a problem hiding this comment.
Why should we disable parallelization?
There was a problem hiding this comment.
There is a nasty thing in the implementation for the spanner .net client that requires an environment variable to be set in case of using the emulator: https://cloud.google.com/spanner/docs/emulator#cs
So when running multiple tests in parrallel, they override eachother environment variable.
So this environment variable is now also set in the startup, As i intended to hide this emulator specific from the client, i want to do a little investigation if there is a better place/way.
There was a problem hiding this comment.
To be more specific the detection setting in the string will make the client require this environment variable, it will not take the one from the connection string.
There was a problem hiding this comment.
Oh, that is unfortunate. Is there an upstream issue? It seems weird that we are unable to connect to multiple instances. Is it possible that we could achieve it by configuring a session pool manager (googleapis/google-cloud-dotnet#4970 (comment))?
There was a problem hiding this comment.
As you can see from the failing tests on ubuntu only, there is a difference in how the dependency on the environment variable is treated in linux and windows, I'll need to gain a more thorough understanding of this before completing this so I'll be trying to find resources/ threads /issues or source code reference on this first.
There was a problem hiding this comment.
What do you think about utilizing an xUnit.net collection fixture instead of disabling parallelization? This creates a single Cloud Spanner instance for the tests. It would not be necessary to update the environment variable anymore.
…able parallelisation because of the environment variable usage
TwanVB
force-pushed
the
feature/862-add-spanner-module
branch
from
0e4d81b
to
37e0c92
Compare
|
Your PR hasn't seen any recent activities, I will close it. Please, do not hesitate to reopen it again if you continue to work on it. |
What does this PR do?
Add spanner (emulator) module:
Why is it important?
Other developers in need of testing locally or in CI processes against a spanner will need less code and time to set up their testcontainer. The testcontainers module set will be bigger, increasing the maturity of the suite.
Related issues