Skip to content

Conversation

@KyleAure
Copy link
Contributor

Allow the Oracle Free container to leverage the use of a RemoteDockerImage by exposing an equivalent constructor as the JdbcDatabaseContainer superclass.

@KyleAure KyleAure requested a review from a team May 13, 2025 21:08
@KyleAure
Copy link
Contributor Author

KyleAure commented Jun 3, 2025

FYI - @eddumelendez

@kiview kiview requested a review from Copilot June 11, 2025 13:57
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Sorry @KyleAure, LGTM 👍

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables the Oracle Free container to be instantiated with a Future<String> image (e.g., a remote Docker image), matching the superclass API and centralizing the waiting strategy in a shared method.

  • Added a new OracleContainer(Future<String> image) constructor
  • Extracted waitingFor setup into a reusable preconfigure() method
  • Updated the existing DockerImageName constructor to call preconfigure()
Comments suppressed due to low confidence (2)

modules/oracle-free/src/main/java/org/testcontainers/oracle/OracleContainer.java:67

  • Please add JavaDoc for this constructor to explain its purpose and expected behavior when supplying a Future image.
public OracleContainer(Future<String> image) {

modules/oracle-free/src/main/java/org/testcontainers/oracle/OracleContainer.java:67

  • There are no existing tests for this new constructor; please add unit tests that resolve the Future and verify the waiting strategy is applied correctly.
public OracleContainer(Future<String> image) {

this(DockerImageName.parse(dockerImageName));
}

public OracleContainer(Future<String> image) {
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

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

The new Future constructor does not enforce compatibility with DEFAULT_IMAGE_NAME. Consider parsing the resolved image into a DockerImageName and calling assertCompatibleWith(DEFAULT_IMAGE_NAME) to maintain consistent safety checks.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

That comment is actually correct.

Comment on lines +69 to +78
preconfigure();
}

public OracleContainer(final DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
preconfigure();
}

private void preconfigure() {
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The method name preconfigure is generic; consider renaming it to something more descriptive, like applyDefaultWaitingStrategy or configureStartupWait for clarity.

Suggested change
preconfigure();
}
public OracleContainer(final DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
preconfigure();
}
private void preconfigure() {
configureStartupWait();
}
public OracleContainer(final DockerImageName dockerImageName) {
super(dockerImageName);
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME);
configureStartupWait();
}
private void configureStartupWait() {

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

I think we normally have this config in a protected void configure() method.

@kiview kiview self-requested a review June 11, 2025 14:00
@eddumelendez
Copy link
Member

Hi @KyleAure, I think there was an oversight on the first Oracle-XE implementation given that other Testcontainers implementations don't support it. You may want to try something like it is described here

@KyleAure
Copy link
Contributor Author

KyleAure commented Aug 4, 2025

@eddumelendez
I would argue that more Testcontainer implementations should have support for the RemoteDockerImage constructor instead of closing this PR saying that no other implementations use it.
If I were to make a PR to add support for a RemoteDockerImage constructor for all implementations, and added the ability for RemoteDockerImage enforce compatibility with DEFAULT_IMAGE_NAME, would that be accepted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants