Conversation
Allows database state to be reused across restarts, and more generally makes startup faster. Signed-off-by: nscuro <nscuro@protonmail.com>
There was a problem hiding this comment.
Pull request overview
Adds support for reusing Testcontainers-backed “dev services” containers across API server restarts, aiming to speed up local development by keeping stateful containers (e.g., PostgreSQL) running.
Changes:
- Introduces a new configuration flag to enable/disable dev-services container reuse.
- Configures PostgreSQL/Kafka/frontend dev-service containers with labels and
withReuse(...). - Adjusts Kafka topic creation to tolerate existing topics when reusing containers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apiserver/src/main/resources/application.properties | Adds dev.services.container-reuse-enabled configuration and docs for container reuse. |
| apiserver/src/main/java/org/dependencytrack/dev/DevServicesInitializer.java | Enables container reuse/labels, tolerates “topic already exists”, and conditionally skips stopping containers on shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Defines whether dev services containers should be reused. | ||
| # <br/><br/> | ||
| # Refer to <https://java.testcontainers.org/features/reuse/> for details. | ||
| # | ||
| # @category: Development | ||
| # @type: boolean | ||
| dev.services.container-reuse-enabled=true |
There was a problem hiding this comment.
The new dev.services.container-reuse-enabled setting is documented, but it doesn’t mention two important behavioral constraints of Testcontainers reuse: (1) reuse is ignored unless TESTCONTAINERS_REUSE_ENABLE=true or testcontainers.reuse.enable=true is set, and (2) when reuse is enabled the containers are intentionally not disposed on shutdown. As written, this also conflicts with the earlier dev-services description that says containers are disposed when Dependency-Track stops. Please update the property docs (and/or the dev-services section above) to reflect the prerequisite and the changed shutdown behavior so users don’t end up with unexpected lingering containers.
| public void contextDestroyed(final ServletContextEvent event) { | ||
| if (postgresContainer != null) { | ||
| if (postgresContainer != null && !isContainerReuseEnabled) { | ||
| LOGGER.info("Stopping postgres container"); | ||
| try { | ||
| postgresContainer.close(); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Failed to stop PostgreSQL container", e); | ||
| } | ||
| } | ||
| if (kafkaContainer != null) { | ||
| if (kafkaContainer != null && !isContainerReuseEnabled) { | ||
| LOGGER.info("Stopping Kafka container"); | ||
| try { | ||
| kafkaContainer.close(); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Failed to stop Kafka container", e); | ||
| } | ||
| } | ||
| if (frontendContainer != null) { | ||
| if (frontendContainer != null && !isContainerReuseEnabled) { | ||
| LOGGER.info("Stopping frontend container"); |
There was a problem hiding this comment.
contextDestroyed() skips closing containers solely based on dev.services.container-reuse-enabled. In Testcontainers, withReuse(true) has no effect unless reuse is enabled globally (e.g. testcontainers.reuse.enable=true), so this can leave non-reusable containers running (and keep host ports bound) when the webapp is stopped/redeployed without a JVM exit. Consider only skipping close() when Testcontainers reports that the environment supports reuse, otherwise always close containers on shutdown.
| } catch (ExecutionException | InterruptedException e) { | ||
| throw new RuntimeException("Failed to create topics", e); | ||
| if (e.getCause() == null | ||
| || !"TopicExistsException".equals(e.getCause().getClass().getSimpleName())) { | ||
| throw new RuntimeException("Failed to create topics", e); | ||
| } |
There was a problem hiding this comment.
The TopicExists handling is based on comparing e.getCause().getClass().getSimpleName() to a string. This is brittle and can misclassify unrelated exceptions with the same simple name; it also doesn’t preserve the thread interrupt status when an InterruptedException is caught. Prefer checking the actual Kafka exception type (e.g. org.apache.kafka.common.errors.TopicExistsException, accounting for potential wrapping) and, for InterruptedException, re-interrupt the thread before propagating/handling it.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Description
Supports reuse of dev containers.
Allows database state to be reused across restarts, and more generally makes startup faster.
Addressed Issue
N/A
Additional Details
N/A
Checklist
This PR fixes a defect, and I have provided tests to verify that the fix is effectiveThis PR implements an enhancement, and I have provided tests to verify that it works as intendedThis PR introduces changes to the database model, and I have updated the migration changelog accordinglyThis PR introduces new or alters existing behavior, and I have updated the documentation accordingly