-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New Oracle FREE support #7624
New Oracle FREE support #7624
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, @ecki! Please add proper tests and docs.
|
||
public static final String NAME = "oracle"; | ||
|
||
private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("container-registry.oracle.com/database/free"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, take into account this image instead. It offers many improvements that are suitable for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to stick to the official image, but maybe we can make the existing Oracle Ntainer accept all of them, instead? (And should in this case the module be renamed or a new one addd with all 3 images?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, the official image is heavy and it takes around 3 min to start. That's not the experience we want to provide. If you are willing to do that then you can opt-in by using
DockerImageName image = DockerImageName.parse("container-registry.oracle.com/database/free").asCompatibleSubstituteFor("gvenzl/oracle-free");
new OracleFreeContainer(image);
/** | ||
* @deprecated use {@link #OracleContainer(DockerImageName)} instead | ||
*/ | ||
@Deprecated | ||
public OracleFreeContainer() { | ||
this(DEFAULT_IMAGE_NAME.withTag(DEFAULT_TAG)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most useful constructor and I am actually using it. Any idea why it is deprecated by the source class I copied? How am I supposed to get the default image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testcontainers recommends to used fixed images. The main reason is because default constructors it will hide what version is using the library. Reading the test code should explain even against what version is being used. In this example, would be new OracleFreeContainer("gvenzl/oracle-free:23.3-slim-faststart")
public OracleFreeContainer(Future<String> dockerImageName) { | ||
super(dockerImageName); | ||
preconfigure(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it, please.
preconfigure(); | ||
} | ||
|
||
private void preconfigure() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a configure()
method that can be overridden so you lines 83 and 88 can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe, I was sticking to the original source, should we fix/change the original class then as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as part of this PR. Another PR would be welcomed.
Hi @ecki, I wonder if you are able to continue working on this PR? Hope everything is doing well. |
Yes but I probably just close it, I am not sure why I made my own copy when in principle only the image name changes. btw free starts really fast, it only needs 4mins for downloads, which is then (hopefully) cached. |
Implement by #7749 (for inofficial image) |
Is this something you want in the existing module or in a new module? Any reason why its not supported yet? Want to add the new Oracle image as a compatibility in the existing OracleContainer instead? (besides the APEX port it seems to work).