feat : use test containers instead of loading database #1494
Conversation
WalkthroughThe changes in this pull request involve modifications to the GitHub Actions workflow, the addition of a new dependency for PostgreSQL Testcontainers in the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
type: dependency-upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/TestReadReplicaApplication.java (1)
6-13: Consider enhancing the test class structure and documentationWhile the implementation is clean, consider the following improvements for better maintainability and clarity:
- Add class-level documentation explaining the purpose and usage of this test class
- Make the class public for better accessibility in the test context
- Consider adding appropriate test annotations if this is intended to be used as a test class
- Consider renaming to follow test class naming conventions (e.g.,
ReadReplicaApplicationTest)Here's a suggested improvement:
+/** + * Test configuration class that sets up the application context with test containers + * for integration testing of read replica functionality. + */ +@SpringBootTest -class TestReadReplicaApplication { +public class ReadReplicaApplicationTest { public static void main(String[] args) { SpringApplication.from(ReadReplicaApplication::main) .with(ContainersConfiguration.class) .run(args); } }jpa/boot-read-replica-postgresql/pom.xml (1)
Line range hint
6-9: Consider using a GA version instead of RC1 for production use.The project is using Spring Boot 3.4.0-RC1 (Release Candidate). While this works for development and testing, it's recommended to use GA (General Availability) versions for production deployments to ensure stability and support.
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java (2)
55-60: Consider enhancing error handling with more specific exceptionsWhile the validation logic is good, consider replacing the generic assertion messages with custom exceptions that provide more context about the configuration issues. This would make debugging easier in case of setup failures.
Example improvement:
private void validateDataSourceHierarchy(DataSource dataSource) { - assertThat(dataSource) - .isNotNull() - .withFailMessage("DataSource must not be null") - .isInstanceOf(LazyConnectionDataSourceProxy.class); + if (dataSource == null) { + throw new IllegalStateException("DataSource configuration is missing. Ensure database properties are correctly set."); + } + if (!(dataSource instanceof LazyConnectionDataSourceProxy)) { + throw new IllegalStateException("Invalid DataSource hierarchy. Expected LazyConnectionDataSourceProxy but found: " + + dataSource.getClass().getSimpleName()); + } }Also applies to: 62-70, 72-81
Line range hint
109-120: Consider making replication timeout configurableThe tests use a hard-coded 3-second timeout for replication verification. Consider making this configurable to accommodate different environments where replication might take longer.
Example improvement:
+ @Value("${test.replication.timeout:3}") + private int replicationTimeoutSeconds; + @Order(3) @Test void shouldInsertViaPrimary() { // ... existing code ... - await().atMost(3, TimeUnit.SECONDS) + await().atMost(replicationTimeoutSeconds, TimeUnit.SECONDS) .untilAsserted(Also applies to: 134-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/jpa-boot-read-replica-postgresql.yml(0 hunks)jpa/boot-read-replica-postgresql/pom.xml(1 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java(3 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/TestReadReplicaApplication.java(1 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/jpa-boot-read-replica-postgresql.yml
🔇 Additional comments (8)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/TestReadReplicaApplication.java (2)
1-5: LGTM! Clean and minimal imports
The imports are well-organized and contain only the necessary dependencies for the test configuration.
9-11: Verify container configuration integration
The integration with ContainersConfiguration looks correct, but let's verify the implementation patterns across the test suite.
✅ Verification successful
Let me check the implementation of ContainersConfiguration to ensure it follows the correct patterns.
Container configuration integration is properly implemented
The ContainersConfiguration implementation follows the correct patterns with:
- Proper
@TestConfigurationannotation and container setup - Well-configured master-slave PostgreSQL replication setup
- Correct dynamic property registration for both datasources
- Similar structure to other container configurations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify container configuration patterns in test classes
# Check ContainersConfiguration implementation
echo "Checking ContainersConfiguration implementation:"
rg -l "class ContainersConfiguration" --type java
# Check similar test container usage patterns
echo "\nChecking test container usage patterns:"
rg -l "SpringApplication.from.*with.*Configuration" --type java
# Check other test classes using test containers
echo "\nChecking other test container usages:"
rg -l "TestContainer" --type java
Length of output: 1619
Script:
#!/bin/bash
# Check the implementation of ContainersConfiguration in the current project
echo "ContainersConfiguration implementation in current project:"
cat jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java
# Check similar implementation in other project for pattern validation
echo -e "\nContainersConfiguration implementation in other project:"
cat jpa/boot-data-multipledatasources/src/test/java/com/example/multipledatasources/common/ContainersConfiguration.java
Length of output: 6126
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java (3)
12-13: LGTM! Well-structured test configuration.
Good use of @TestConfiguration(proxyBeanMethods = false) to optimize bean creation performance since method interception isn't needed in this test context.
62-70: LGTM! Clean implementation of dynamic properties.
Good use of method references for JDBC URLs, ensuring that the latest container state is always used.
1-71: Verify complete migration to test containers.
The implementation looks good, but let's ensure all pieces are in place for the migration:
✅ Verification successful
Let me generate another verification script to check specifically for the read-replica module's configuration:
Migration to test containers is complete and properly configured
The verification confirms successful migration to test containers:
-
Test containers dependencies are correctly configured in pom.xml:
- spring-boot-testcontainers
- testcontainers-junit-jupiter
- testcontainers-postgresql
-
GitHub Actions workflow has been simplified and no longer contains database setup, relying on test containers instead.
-
No traditional database properties are present in the module's configuration files, as the database setup is now fully managed by test containers through the ContainersConfiguration class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete migration to test containers
# Test 1: Check if old database setup is removed from GitHub Actions
echo "Checking for old database setup in GitHub Actions..."
if [ -d ".github/workflows" ]; then
rg -l "postgres" .github/workflows/
fi
# Test 2: Verify test containers dependency in pom.xml
echo "Checking for test containers dependency..."
if [ -f "pom.xml" ]; then
rg "org.testcontainers" pom.xml -A 2
fi
# Test 3: Check for any remaining traditional database configuration
echo "Checking for traditional database configuration..."
fd -e properties -e yaml | xargs rg "spring.(datasource|jpa)"
Length of output: 63252
Script:
#!/bin/bash
# Check configuration specific to read-replica module
# Check for test containers dependency in the module's pom.xml
echo "Checking test containers dependency in read-replica module..."
if [ -f "jpa/boot-read-replica-postgresql/pom.xml" ]; then
rg "testcontainers" jpa/boot-read-replica-postgresql/pom.xml -A 2
fi
# Check for any database configuration in GitHub Actions workflow specific to this module
echo "Checking GitHub Actions workflow for database setup..."
if [ -f ".github/workflows/jpa-boot-read-replica-postgresql.yml" ]; then
cat .github/workflows/jpa-boot-read-replica-postgresql.yml
fi
# Check for any remaining database properties in the module
echo "Checking for database properties in the module..."
fd -e properties -e yaml -p jpa/boot-read-replica-postgresql | xargs rg "spring.(datasource|jpa)"
Length of output: 1913
jpa/boot-read-replica-postgresql/pom.xml (1)
79-83: LGTM! The PostgreSQL Testcontainers dependency is properly configured.
The addition of the PostgreSQL Testcontainers dependency with test scope aligns well with the PR objective to use test containers instead of loading the database directly. This change complements the existing test container dependencies and enables containerized PostgreSQL testing.
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java (2)
7-7: LGTM: Test configuration properly updated for Testcontainers
The changes correctly integrate the ContainersConfiguration class, aligning with the PR's objective of migrating from Docker Compose to Testcontainers.
Also applies to: 27-27
49-53: LGTM: Improved data source validation
The refactored setup method now properly validates the data source hierarchy before proceeding with template setup.
| PostgreSQLContainer<?> masterPostgreSQLContainer() { | ||
| Map<String, String> envMap = new HashMap<>(); | ||
| envMap.put("POSTGRESQL_REPLICATION_MODE", "master"); | ||
| envMap.put("POSTGRESQL_REPLICATION_USER", "repl_user"); | ||
| envMap.put("POSTGRESQL_REPLICATION_PASSWORD", "repl_password"); | ||
| envMap.put("POSTGRESQL_USERNAME", "postgres_write"); | ||
| envMap.put("POSTGRESQL_PASSWORD", "postgres_write"); | ||
| envMap.put("POSTGRESQL_DATABASE", "my_database"); | ||
|
|
||
| PostgreSQLContainer<?> master = | ||
| new PostgreSQLContainer<>( | ||
| DockerImageName.parse("bitnami/postgresql") | ||
| .withTag("latest") | ||
| .asCompatibleSubstituteFor("postgres")) | ||
| .withEnv(envMap) | ||
| .withStartupAttempts(3) | ||
| .withStartupTimeout(Duration.ofMinutes(3)); | ||
| master.start(); | ||
| return master; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improvements for container lifecycle and security.
A few suggestions to enhance the implementation:
- Consider moving credentials to test properties file
- Add container cleanup using
@PreDestroyor register shutdown hook - Consider moving container start to a
@PostConstructmethod instead of during bean definition
Example implementation:
private static final PostgreSQLContainer<?> master = new PostgreSQLContainer<>(/*...*/);
@PostConstruct
void startContainer() {
master.start();
}
@PreDestroy
void stopContainer() {
master.stop();
}
@Bean
PostgreSQLContainer<?> masterPostgreSQLContainer() {
return master;
}| DockerImageName.parse("bitnami/postgresql") | ||
| .withTag("latest") | ||
| .asCompatibleSubstituteFor("postgres")) |
There was a problem hiding this comment.
Avoid using 'latest' tag for test stability.
Using the 'latest' tag can lead to non-deterministic test behavior when the image is updated. Consider pinning to a specific version.
-DockerImageName.parse("bitnami/postgresql")
- .withTag("latest")
+DockerImageName.parse("bitnami/postgresql")
+ .withTag("14.5.0")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DockerImageName.parse("bitnami/postgresql") | |
| .withTag("latest") | |
| .asCompatibleSubstituteFor("postgres")) | |
| DockerImageName.parse("bitnami/postgresql") | |
| .withTag("14.5.0") | |
| .asCompatibleSubstituteFor("postgres")) |
| PostgreSQLContainer<?> slavePostgreSQLContainer( | ||
| PostgreSQLContainer<?> masterPostgreSQLContainer) { | ||
| Map<String, String> envMap = new HashMap<>(); | ||
| envMap.put("POSTGRESQL_REPLICATION_MODE", "slave"); | ||
| envMap.put("POSTGRESQL_REPLICATION_USER", "repl_user"); | ||
| envMap.put("POSTGRESQL_REPLICATION_PASSWORD", "repl_password"); | ||
| envMap.put("POSTGRESQL_USERNAME", "postgres_write"); | ||
| envMap.put("POSTGRESQL_PASSWORD", "postgres_write"); | ||
| envMap.put("POSTGRESQL_MASTER_HOST", masterPostgreSQLContainer.getHost()); | ||
| envMap.put( | ||
| "POSTGRESQL_MASTER_PORT_NUMBER", | ||
| String.valueOf(masterPostgreSQLContainer.getMappedPort(5432))); | ||
|
|
||
| PostgreSQLContainer<?> slave = | ||
| new PostgreSQLContainer<>( | ||
| DockerImageName.parse("bitnami/postgresql:latest") | ||
| .asCompatibleSubstituteFor("postgres")) | ||
| .withEnv(envMap) | ||
| .withStartupAttempts(3) | ||
| .withStartupTimeout(Duration.ofMinutes(3)); | ||
| slave.start(); | ||
| return slave; | ||
| } |
There was a problem hiding this comment.
Add readiness check for master-slave replication.
The slave container might fail to start if the master isn't fully ready for replication. Consider adding a wait strategy to ensure the master is ready before starting the slave.
PostgreSQLContainer<?> slavePostgreSQLContainer(
PostgreSQLContainer<?> masterPostgreSQLContainer) {
+ // Ensure master is ready for replication
+ if (!masterPostgreSQLContainer.isRunning()) {
+ throw new IllegalStateException("Master container must be running before starting slave");
+ }
+ // Wait for master to be ready for replication
+ masterPostgreSQLContainer
+ .withLogConsumer(outputFrame -> {
+ if (outputFrame.getUtf8String().contains("database system is ready to accept connections")) {
+ // Master is ready
+ }
+ });
Map<String, String> envMap = new HashMap<>();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java (2)
62-70: Consider adding comprehensive datasource properties.While the current implementation registers JDBC URLs, consider adding other essential datasource properties:
@Bean public DynamicPropertyRegistrar databaseProperties( PostgreSQLContainer<?> masterPostgreSQLContainer, PostgreSQLContainer<?> slavePostgreSQLContainer) { return (properties) -> { properties.add("spring.primary.datasource.url", masterPostgreSQLContainer::getJdbcUrl); + properties.add("spring.primary.datasource.username", masterPostgreSQLContainer::getUsername); + properties.add("spring.primary.datasource.password", masterPostgreSQLContainer::getPassword); properties.add("spring.replica.datasource.url", slavePostgreSQLContainer::getJdbcUrl); + properties.add("spring.replica.datasource.username", slavePostgreSQLContainer::getUsername); + properties.add("spring.replica.datasource.password", slavePostgreSQLContainer::getPassword); }; }
1-71: Consider implementing container lifecycle management and reusability.To improve the overall architecture:
- Consider implementing
@DynamicPropertySourceinstead ofDynamicPropertyRegistrarfor better Spring Test integration.- Add container cleanup by implementing
DisposableBeanto ensure proper resource cleanup.- Consider extracting container configurations to a separate properties class for reusability.
Example implementation:
@TestConfiguration(proxyBeanMethods = false) public class ContainersConfiguration implements DisposableBean { private final List<PostgreSQLContainer<?>> containers = new ArrayList<>(); // ... existing methods ... @Override public void destroy() { containers.forEach(PostgreSQLContainer::stop); } }jpa/boot-read-replica-postgresql/pom.xml (1)
Update Spring Boot version to 3.3.5 for improved stability
The latest stable version of Spring Boot 3.x is 3.3.5, which is recommended over the current release candidate version 3.4.0-RC1. Using a stable release ensures better reliability and support in production environments.
- Replace
<version>3.4.0-RC1</version>with<version>3.3.5</version>inpom.xml🔗 Analysis chain
Line range hint
6-9: Consider using a stable version of Spring Boot.Currently using
3.4.0-RC1which is a release candidate. For production applications, it's recommended to use stable releases to avoid potential issues.Let's check if a stable version is available:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest stable version of Spring Boot 3.x gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/spring-projects/spring-boot/releases | \ jq -r '.[] | select(.prerelease==false) | .tag_name' | \ grep -E "^v3" | head -n1Length of output: 220
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java (2)
49-70: Enhance error messages for better debugging.While the validation logic is solid, consider making the error messages more descriptive by including actual vs expected types. This would make debugging easier when tests fail.
Example improvement:
- .withFailMessage("Target DataSource must not be null") + .withFailMessage("Target DataSource must not be null. Got: %s, Expected: %s", + targetDataSource != null ? targetDataSource.getClass().getSimpleName() : "null", + ProxyDataSource.class.getSimpleName())
Consider increasing the await timeout for replica synchronization tests
The current 3-second timeout is too aggressive for PostgreSQL replication tests. The docker-compose configuration shows a master-slave replication setup that requires time for:
- Initial container startup
- Replication connection establishment
- Data synchronization between master and replica
Recommended changes:
- await().atMost(3, TimeUnit.SECONDS) + await().atMost(30, TimeUnit.SECONDS) + .pollInterval(Duration.ofSeconds(2))The increased timeout and poll interval will:
- Better accommodate container startup and replication lag
- Reduce test flakiness in CI environments
- Match patterns seen in other replication tests in the codebase
🔗 Analysis chain
Line range hint
109-123: Consider increasing the await timeout for container-based tests.The current 3-second timeout might be too aggressive for container-based tests, especially when considering container startup time and replication lag. This could lead to flaky tests in CI environments.
- await().atMost(3, TimeUnit.SECONDS) + await().atMost(10, TimeUnit.SECONDS) .pollInterval(1, TimeUnit.SECONDS)Let's verify the container startup times:
Also applies to: 134-148
Let me analyze the test infrastructure and timing patterns to provide a more informed response about the timeout values.
Let me analyze the container setup and timing patterns to provide a more informed response about the timeout values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check container startup logs to analyze timing patterns rg -A 5 "Starting.*container" .Length of output: 33
Script:
#!/bin/bash # Let's check for test configuration files and container setup fd -e yaml -e properties -e xmlLength of output: 22568
Script:
#!/bin/bash # Search for container configuration and timing patterns in test files rg -l "testcontainers|@Container|@ServiceConnection" .Length of output: 7938
Script:
#!/bin/bash # Look for actual test execution times or timing configurations rg "await\(\)|atMost|pollInterval|setTimeout|duration" .Length of output: 278300
Script:
#!/bin/bash # Let's check the test container configuration and startup details rg -l "testcontainers|@Container" jpa/boot-read-replica-postgresql/Length of output: 222
Script:
#!/bin/bash # Look for container configuration and startup settings cat jpa/boot-read-replica-postgresql/docker-compose.yamlLength of output: 1410
Script:
#!/bin/bash # Check for any existing timing configurations in the test rg "await|atMost|duration|pollInterval" jpa/boot-read-replica-postgresql/Length of output: 537
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/jpa-boot-read-replica-postgresql.yml(0 hunks)jpa/boot-read-replica-postgresql/pom.xml(1 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java(3 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/TestReadReplicaApplication.java(1 hunks)jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/jpa-boot-read-replica-postgresql.yml
🔇 Additional comments (5)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/TestReadReplicaApplication.java (2)
8-12: LGTM! Main method implementation is well structured
The main method follows Spring Boot's best practices:
- Correctly references the main application using method reference
- Properly integrates container configuration
- Uses the builder pattern effectively
3-3: Verify ContainersConfiguration implementation
The ContainersConfiguration class is crucial for setting up PostgreSQL test containers. Let's verify its implementation to ensure proper container configuration.
✅ Verification successful
Let me check the specific ContainersConfiguration implementation for the read replica setup.
ContainersConfiguration is properly implemented for read replica setup
The ContainersConfiguration class is correctly implemented with:
- Master PostgreSQL container with proper replication configuration
- Slave PostgreSQL container linked to master for replication
- Dynamic property registration for both primary and replica datasources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ContainersConfiguration class implementation
# Expected: Should find container setup with proper configuration for primary and replica databases
# Search for ContainersConfiguration class implementation
ast-grep --pattern 'class ContainersConfiguration {
$$$
}'
# Search for PostgreSQL container configuration
rg -l "PostgreSQLContainer" --type java
Length of output: 2703
Script:
#!/bin/bash
# Get the implementation of ContainersConfiguration for read replica
cat "jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java"
Length of output: 3251
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/common/ContainersConfiguration.java (1)
1-13: LGTM! Well-structured configuration class.
The class is properly configured with @TestConfiguration(proxyBeanMethods = false), which is the recommended approach for test configurations where bean proxying isn't needed.
jpa/boot-read-replica-postgresql/pom.xml (1)
79-83: LGTM! Testcontainers dependency is properly configured.
The PostgreSQL Testcontainers dependency is correctly added with test scope, which aligns with the PR objective of switching from Docker Compose to Testcontainers for database testing.
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/ReadReplicaApplicationTests.java (1)
7-7: LGTM! Good integration of test containers.
The changes properly integrate the container configuration into the test context, making the test infrastructure more explicit and self-contained.
Also applies to: 27-27
| class TestReadReplicaApplication { | ||
|
|
||
| public static void main(String[] args) { | ||
| SpringApplication.from(ReadReplicaApplication::main) | ||
| .with(ContainersConfiguration.class) | ||
| .run(args); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance class structure and documentation
The class structure could be improved for better maintainability and clarity.
Apply these improvements:
+/**
+ * Test application class that configures and runs the application with TestContainers support.
+ * This class replaces the need for Docker Compose by using Testcontainers for database setup.
+ */
-class TestReadReplicaApplication {
+public final class TestReadReplicaApplication {
+
+ private TestReadReplicaApplication() {
+ // Prevents instantiation
+ }
public static void main(String[] args) {
SpringApplication.from(ReadReplicaApplication::main)
.with(ContainersConfiguration.class)
.run(args);
}
}The changes include:
- Added class-level documentation explaining its purpose
- Made the class public and final
- Added private constructor to prevent instantiation
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestReadReplicaApplication { | |
| public static void main(String[] args) { | |
| SpringApplication.from(ReadReplicaApplication::main) | |
| .with(ContainersConfiguration.class) | |
| .run(args); | |
| } | |
| } | |
| /** | |
| * Test application class that configures and runs the application with TestContainers support. | |
| * This class replaces the need for Docker Compose by using Testcontainers for database setup. | |
| */ | |
| public final class TestReadReplicaApplication { | |
| private TestReadReplicaApplication() { | |
| // Prevents instantiation | |
| } | |
| public static void main(String[] args) { | |
| SpringApplication.from(ReadReplicaApplication::main) | |
| .with(ContainersConfiguration.class) | |
| .run(args); | |
| } | |
| } |
| @Bean | ||
| PostgreSQLContainer<?> masterPostgreSQLContainer() { | ||
| Map<String, String> envMap = new HashMap<>(); | ||
| envMap.put("POSTGRESQL_REPLICATION_MODE", "master"); | ||
| envMap.put("POSTGRESQL_REPLICATION_USER", "repl_user"); | ||
| envMap.put("POSTGRESQL_REPLICATION_PASSWORD", "repl_password"); | ||
| envMap.put("POSTGRESQL_USERNAME", "postgres_write"); | ||
| envMap.put("POSTGRESQL_PASSWORD", "postgres_write"); | ||
| envMap.put("POSTGRESQL_DATABASE", "my_database"); | ||
|
|
||
| PostgreSQLContainer<?> master = | ||
| new PostgreSQLContainer<>( | ||
| DockerImageName.parse("bitnami/postgresql") | ||
| .withTag("latest") | ||
| .asCompatibleSubstituteFor("postgres")) | ||
| .withEnv(envMap) | ||
| .withStartupAttempts(3) | ||
| .withStartupTimeout(Duration.ofMinutes(3)); | ||
| master.start(); | ||
| return master; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider stability and security improvements in master container configuration.
Several improvements could enhance the reliability and security of the test container:
- Replace 'latest' tag with a specific version for test stability:
- .withTag("latest")
+ .withTag("15.5.0") // or another specific version- Consider externalizing test credentials to test properties:
- envMap.put("POSTGRESQL_USERNAME", "postgres_write");
- envMap.put("POSTGRESQL_PASSWORD", "postgres_write");
+ envMap.put("POSTGRESQL_USERNAME", testProperties.getUsername());
+ envMap.put("POSTGRESQL_PASSWORD", testProperties.getPassword());- The startup timeout of 3 minutes seems excessive for most CI environments:
- .withStartupTimeout(Duration.ofMinutes(3));
+ .withStartupTimeout(Duration.ofSeconds(30));Committable suggestion skipped: line range outside the PR's diff.
| @Bean | ||
| PostgreSQLContainer<?> slavePostgreSQLContainer( | ||
| PostgreSQLContainer<?> masterPostgreSQLContainer) { | ||
| Map<String, String> envMap = new HashMap<>(); | ||
| envMap.put("POSTGRESQL_REPLICATION_MODE", "slave"); | ||
| envMap.put("POSTGRESQL_REPLICATION_USER", "repl_user"); | ||
| envMap.put("POSTGRESQL_REPLICATION_PASSWORD", "repl_password"); | ||
| envMap.put("POSTGRESQL_USERNAME", "postgres_write"); | ||
| envMap.put("POSTGRESQL_PASSWORD", "postgres_write"); | ||
| envMap.put("POSTGRESQL_MASTER_HOST", masterPostgreSQLContainer.getHost()); | ||
| envMap.put( | ||
| "POSTGRESQL_MASTER_PORT_NUMBER", | ||
| String.valueOf(masterPostgreSQLContainer.getMappedPort(5432))); | ||
|
|
||
| PostgreSQLContainer<?> slave = | ||
| new PostgreSQLContainer<>( | ||
| DockerImageName.parse("bitnami/postgresql:latest") | ||
| .asCompatibleSubstituteFor("postgres")) | ||
| .withEnv(envMap) | ||
| .withStartupAttempts(3) | ||
| .withStartupTimeout(Duration.ofMinutes(3)); | ||
| slave.start(); | ||
| return slave; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential race conditions and configuration consistency in slave container.
The slave container configuration has similar issues to the master container, plus some additional concerns:
- Use the same image version variable for both containers:
- DockerImageName.parse("bitnami/postgresql:latest")
+ DockerImageName.parse("bitnami/postgresql")
+ .withTag("15.5.0")- Add a health check to ensure the master is ready for replication:
+ masterPostgreSQLContainer.waitingFor(
+ Wait.forLogMessage(".*database system is ready to accept connections.*", 2));- Consider adding a replication status check before returning the slave container:
slave.start();
+ // Wait for replication to be established
+ slave.waitingFor(
+ Wait.forLogMessage(".*started streaming WAL from primary.*", 1));
return slave;Committable suggestion skipped: line range outside the PR's diff.
|
Can't be achieved as per testcontainers issue |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor