-
-
Notifications
You must be signed in to change notification settings - Fork 291
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
feat: Update CosmosDb image to vnext-preview version #1324
base: develop
Are you sure you want to change the base?
feat: Update CosmosDb image to vnext-preview version #1324
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the PR. Have you seen this discussion? I would like to wait until an official version is published. In addition, we do not use static ports. While GetAvailablePort()
does not return a static port per se, it may fail for remote container runtimes (it does not resolve the mapped port from the container host).
For internal routing to work as expected, the API port and mapping port need to be configured with the same value. References.
According to the discussion, this is unnecessary, and it is why we used the DelegatingHandler
in the past.
I think we can prepare a PR and merge it as soon as the official version is published, but I favor and would like to implement the proposed changes from the discussion.
@HofmeisterAn Thanks for your comment. I hadn't come across that discussion. I only checked if there were any open issues. I ended up commenting there in the discussion as well. Looking at the discussion, it seems that using the rewriting of the DelegatingHandler is still the best approach, as it also removes the need to know in advance the port where the container will be randomly opened. Even so, some workarounds, like bypassing the certificate, are no longer necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm waiting for this PR because currently, it takes forever to start the image :)
@@ -4,7 +4,7 @@ namespace Testcontainers.CosmosDb; | |||
[PublicAPI] | |||
public sealed class CosmosDbBuilder : ContainerBuilder<CosmosDbBuilder, CosmosDbContainer, CosmosDbConfiguration> | |||
{ | |||
public const string CosmosDbImage = "mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator:latest"; | |||
public const string CosmosDbImage = "mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator:vnext-preview"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use the preview version as a default? I know that this image is quite new but users can always:
new CosmosDbBuilder()
.WithImage("mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator:vnext-preview")
.Build();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of this PR is the use of this image as default, as it delivers significantly better results compared to the current :latest. Additionally, some configurations made in this PR are not compatible between the :latest and :vnext-preview versions. Currently, the :vnext-preview version shows considerably better results than :latest.
new() | ||
{ | ||
ConnectionMode = ConnectionMode.Gateway, | ||
HttpClientFactory = () => new(new UriRewriter(Hostname, GetMappedPublicPort(CosmosDbBuilder.CosmosDbPort))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this.HttpClient here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the discussion here, it was agreed that it would be better to remove the dependency on the Cosmos Client and delegate the responsibility of creating the client to the consumer.
You can already use the |
What does this PR do?
This PR updates the CosmosDb image to the new version Microsoft is working on
Linux-based emulator (preview)
GitHub Repository
Why is it important?
This new image significantly improves the container startup process, eliminates certificate-related workarounds, and integrates seamlessly with CI/CD pipelines like GitHub Actions. For reference, you can check some of my tests here and GitActions here.
Related issues
#1306
Details
Explorer UI Disabled
The explorer UI was disabled since the container's purpose is to test integration with the Cosmos DB API.
Internal Routing Configuration
For internal routing to work as expected, the API port and mapping port need to be configured with the same value. References.
CosmosClient Configuration
The CosmosClient must be configured to use Gateway mode. See the official guidance.