Skip to content
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

Add Db2 module #1237

Open
wants to merge 52 commits into
base: develop
Choose a base branch
from
Open

Add Db2 module #1237

wants to merge 52 commits into from

Conversation

kevin0x90
Copy link

What does this PR do?

Adds a module to use Db2 with testcontainers.

How to test this PR

Automated tests have been added to the project that verify startup and database connectivity.

Copy link

netlify bot commented Aug 20, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 61bfd3d
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/66c45ad21aeb910008c69a02
😎 Deploy Preview https://deploy-preview-1237--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented Aug 20, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit d4f83d2
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/67a76d39d45e380008d3b9a4
😎 Deploy Preview https://deploy-preview-1237--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

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 HofmeisterAn added enhancement New feature or request module An official Testcontainers module labels Aug 29, 2024
@kevin0x90 kevin0x90 marked this pull request as ready for review September 9, 2024 20:55
@HofmeisterAn HofmeisterAn changed the base branch from develop to main January 27, 2025 19:31
@HofmeisterAn HofmeisterAn changed the base branch from main to develop January 27, 2025 19:31
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR and your contribution! I apologize for the delayed review and feedback. I just hadn't found the time earlier. I have a few minor change requests (or questions), but nothing major. I'm happy to merge the PR once those are addressed 🙏.

.WithDatabase(DefaultDatabase)
.WithUsername(DefaultUsername)
.WithPassword(DefaultPassword)
.WithLicenseAgreement()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use the same pattern as we do in the ServiceBus module: 1), 2)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will adjust

Comment on lines +76 to +96
/// <summary>
/// Sets the Db2 archive logs.
/// </summary>
/// <param name="archiveLogs">The Db2 archive logs setting.</param>
/// <returns>A configured instance of <see cref="Db2Builder" />.</returns>
public Db2Builder WithArchiveLogs(bool archiveLogs)
{
return Merge(DockerResourceConfiguration, new Db2Configuration(archiveLogs: archiveLogs))
.WithEnvironment("ARCHIVE_LOGS", archiveLogs.ToString());
}

/// <summary>
/// Sets the Db2 autoconfig setting.
/// </summary>
/// <param name="autoConfig">The Db2 autoconfig setting.</param>
/// <returns>A configured instance of <see cref="Db2Builder" />.</returns>
public Db2Builder WithAutoconfig(bool autoConfig)
{
return Merge(DockerResourceConfiguration, new Db2Configuration(autoConfig: autoConfig))
.WithEnvironment("AUTOCONFIG", autoConfig.ToString());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to provide an API for these configurations? Usually, modules only include what's necessary to run the container; we do not provide an API for every single configuration. Developers can always configure the environment variables themselves (if they are optional).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it makes these settings more discoverable for users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our experience providing APIs (except when necessary to start the container), for configurations that can simply be set through environment variables have several disadvantages:

  • Depending on the number of configurations, providing APIs for all of them requires significant effort.
  • Distinguishing between different image versions and their respective configurations is difficult.
  • We need to include and maintain documentation.
  • Keeping the APIs and documentations up to date.

Testcontainers' modules are opinionated, pre-configured, and follow best practice configurations. A module does not need to support all possible use cases. If developers require a specific configuration, they can always use the generic builder (ContainerBuilder()).

If certain configurations are useful for development and testing, we should set them by default, as we do for PostgreSQL. If the APIs are not necessary, I recommend removing them.

src/Testcontainers.Db2/Db2Builder.cs Show resolved Hide resolved
/// <returns>A configured instance of <see cref="Db2Builder" />.</returns>
public Db2Builder WithInMemoryDatabase()
{
return Merge(DockerResourceConfiguration, new Db2Configuration(licenseAgreement: DefaultInMemoryDatabasePath))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line passes the value to the wrong argument.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will adjust

Comment on lines +82 to +115
/// <summary>
/// Gets the Db2 database.
/// </summary>
public string Database { get; }

/// <summary>
/// Gets the Db2 username.
/// </summary>
public string Username { get; }

/// <summary>
/// Gets the Db2 password.
/// </summary>
public string Password { get; }

/// <summary>
/// Toggle for archivation of logs.
/// </summary>
public bool ArchiveLogs { get; }

/// <summary>
/// Toggle for database autoconfiguration.
/// </summary>
public bool AutoConfig { get; }

/// <summary>
/// License agreement value.
/// </summary>
public string LicenseAgreement { get; }

/// <summary>
/// Path to the database files that should be mapped into memory (tmpfs).
/// </summary>
public string InMemoryDatabasePath { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only include the properties in the configuration type that we will need later in the container type.

[PublicAPI]
public sealed class Db2Builder : ContainerBuilder<Db2Builder, Db2Container, Db2Configuration>
{
public const string Db2Image = "icr.io/db2_community/db2:latest";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pin the version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will adjust

Comment on lines +123 to +131
// By default, the base builder waits until the container is running. However, for Db2, a more advanced waiting strategy is necessary
// If the user does not provide a custom waiting strategy, append the default Db2 waiting strategy.
var db2Builder = DockerResourceConfiguration.WaitStrategies.Count() > 1
? this
: WithWaitStrategy(Wait.ForUnixContainer()
.UntilMessageIsLogged("All databases are now active")
.UntilMessageIsLogged("Setup has completed.")
.AddCustomWaitStrategy(new WaitUntil(DockerResourceConfiguration))
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the wait strategy configuration to Init(). We do not need the pattern here. We only need it if we need to configure the wait strategy with developer-provided configurations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module An official Testcontainers module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants