Feature/862 add spanner module #865

TwanVB · 2023-04-03T21:14:09Z

What does this PR do?

Add spanner (emulator) module:

Created a new module based on the template
Chose to use rest for internal setup of the container as to not create dependency issues when a client under test would depend upon a different version of the google spanner client Google.Cloud.Spanner.Data

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

Closes [Enhancement]: Add spanner (emulator) module #862

netlify · 2023-04-03T21:14:15Z

✅ Deploy Preview for testcontainers-dotnet ready!

Name	Link
🔨 Latest commit	`37e0c92`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-dotnet/deploys/645d7032107066000820d189
😎 Deploy Preview	https://deploy-preview-865--testcontainers-dotnet.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

HofmeisterAn

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?

src/Testcontainers.Spanner/.editorconfig

src/Testcontainers.Spanner/SpannerBuilder.cs

src/Testcontainers.Spanner/SpannerContainer.cs

HofmeisterAn · 2023-04-04T14:13:24Z

src/Testcontainers.Spanner/SpannerContainer.cs

+  {
+    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();
+  }


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.

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.

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.

HofmeisterAn · 2023-04-04T14:20:34Z

src/Testcontainers.Spanner/SpannerContainer.cs

+    {
+      await Task.Delay(TimeSpan.FromSeconds(delayInSeconds));
+      string operationName = operation.Name;
+      var operationResponse = await _webClient.GetAsync($"v1/{operationName}").ValidateAsync("get operation");


You can use the HttpWaitStrategy to wait for a specific HTTP response.

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.

src/Testcontainers.Spanner/Testcontainers.Spanner.csproj

tests/Testcontainers.Spanner.Tests/.editorconfig

HofmeisterAn · 2023-04-04T14:24:36Z

tests/Testcontainers.Spanner.Tests/Properties/AssemblyInfo.cs

@@ -0,0 +1 @@
+[assembly: CollectionBehavior(DisableTestParallelization = true)]


Why should we disable parallelization?

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.

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.

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))?

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.

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

HofmeisterAn · 2023-09-28T16:52:45Z

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.

HofmeisterAn requested changes Apr 4, 2023

View reviewed changes

TwanVB added 4 commits May 11, 2023 09:40

feature 862 Add intial spanner module as startingpoint

7680ef8

feat: Test project enhanced

51a5b00

feat: Process the simple review comments

587fb9c

Process further review comments and add demo for the necessity of dis…

37e0c92

…able parallelisation because of the environment variable usage

TwanVB force-pushed the feature/862-add-spanner-module branch from 0e4d81b to 37e0c92 Compare May 11, 2023 22:46

HofmeisterAn closed this Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/862 add spanner module #865

Feature/862 add spanner module #865

TwanVB commented Apr 3, 2023 •

edited

Loading

netlify bot commented Apr 3, 2023 •

edited

Loading

HofmeisterAn left a comment •

edited

Loading

HofmeisterAn Apr 4, 2023 •

edited

Loading

TwanVB Apr 8, 2023

HofmeisterAn Apr 12, 2023

HofmeisterAn Apr 4, 2023

TwanVB Apr 8, 2023

HofmeisterAn Apr 4, 2023

TwanVB Apr 8, 2023 •

edited

Loading

TwanVB Apr 8, 2023

HofmeisterAn Apr 12, 2023

TwanVB May 12, 2023 •

edited

Loading

HofmeisterAn May 15, 2023

HofmeisterAn commented Sep 28, 2023

		@@ -0,0 +1 @@
		[assembly: CollectionBehavior(DisableTestParallelization = true)]

Feature/862 add spanner module #865

Feature/862 add spanner module #865

Conversation

TwanVB commented Apr 3, 2023 • edited Loading

What does this PR do?

Why is it important?

Related issues

netlify bot commented Apr 3, 2023 • edited Loading

✅ Deploy Preview for testcontainers-dotnet ready!

HofmeisterAn left a comment • edited Loading

Choose a reason for hiding this comment

HofmeisterAn Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TwanVB Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TwanVB May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HofmeisterAn commented Sep 28, 2023

TwanVB commented Apr 3, 2023 •

edited

Loading

netlify bot commented Apr 3, 2023 •

edited

Loading

HofmeisterAn left a comment •

edited

Loading

HofmeisterAn Apr 4, 2023 •

edited

Loading

TwanVB Apr 8, 2023 •

edited

Loading

TwanVB May 12, 2023 •

edited

Loading