feat: MsSql => Turning `WithDatabase` into a public method

testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.

https://dotnet.testcontainers.org

MIT License

3.65k stars 250 forks source link

feat: MsSql => Turning `WithDatabase` into a public method #1141

Closed henriqueholtz closed 3 months ago

henriqueholtz commented 3 months ago

What does this PR do?

Turning MsSqlBuilder.WithDatabase into a public method.

Why is it important?

This is important to set up the default database.

Note: This method is already public on Testcontainers.PostgreSql for example.

Without this method as public, I needed to implement the IAsyncLifetime, and use the following trick:

public async Task InitializeAsync()
{
     await _dbContainer.StartAsync().ConfigureAwait(false);
     await _dbContainer.ExecScriptAsync("CREATE DATABASE [MyDatabase]");
     await _dbContainer.ExecScriptAsync("ALTER LOGIN myuser WITH DEFAULT DATABASE = [MyDatabase]");
}

Related issues

N/A

How to test this PR

Just run the existing tests (specially Testcontainers.MsSql.Tests) and verify that nothing is broken.

netlify[bot] commented 3 months ago

Deploy Preview for testcontainers-dotnet ready!

Name	Link
Latest commit	6b03f137af1e29951c98ceed8e54eadeae5b4f4c
Latest deploy log	https://app.netlify.com/sites/testcontainers-dotnet/deploys/65f419395edac40008ef1955
Deploy Preview	https://deploy-preview-1141--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 configuration.

HofmeisterAn commented 3 months ago

I am sorry, but it is not that trivial (the MSSQL module won't start at all). Simply making WithDatabase(string) public is not enough. There are more changes necessary. Please take a look at the following comment (https://github.com/testcontainers/testcontainers-dotnet/discussions/828#discussioncomment-7589958) and the referred comments/issues. If you are considering contributing (reviewing the proposal) and need further help, please do not hesitate to reopen the PR. I am happy to assist with guidance and reviews.

henriqueholtz commented 3 months ago

I am sorry, but it is not that trivial (the MSSQL module won't start at all). Simply making WithDatabase(string) public is not enough. There are more changes necessary. Please take a look at the following comment (#828 (reply in thread)) and the referred comments/issues. If you are considering contributing (reviewing the proposal) and need further help, please do not hesitate to reopen the PR. I am happy to assist with guidance and reviews.

@HofmeisterAn I took a look in the thread. What about if I implement the scripts used in this PR's description? Maybe creating a new method like WithCustomDatabase(string) or something like that. I just want to be sure if it would be a worthwhile PR.

It's not a native implementation but it would turn out exactly what you declared as a must here:

{...} The instances should be immediately usable without relying on additional user configurations, setups, or frameworks {....}