-
-
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
adds OrientDB 3.0.x module #1414
Conversation
bumps orientdb version to 3.0.17
Hey @robfrank, Since there isn't that much actual OrientDB implementation code inside the module, I wonder if this makes really sense as its own module. WDYT about having this as an example + docs in |
Sorry for my long silence. I developed the module for two main reasons:
BTW, I understand that having a great number of modules isn't simply manageable, so I can move the code inside our repo under the com.arcadeanalytics group id and improve the examples/doc on testocontainers to show how to use OrientDB with GenericContainer (that is the way we use it right now) Or, just merge this :) WDYT? |
I think this probably passes the threshold of adding sufficient value to merit a module of its own. It would be a rather long-winded example snippet. The remaining sometimes-troublesome area is the reliability of the docker image, drivers and avoidance of flakiness. Hopefully this module is pretty reliable on that front (I'll talk to @bsideup and @kiview about managing this). Let's review on the basis that it can be accepted as a module, and see where we are. |
Hi, OrientDB is released frequently, at least once a month in this 2019: https://github.com/orientechnologies/orientdb/releases I can bind the module to "latest" tag, if you think it could be better. |
FWIW I've raised this ticket around the current gap in our docs for new module contributors. I'd like to have a bit of a think about the third point specifically before merging this, but otherwise I'm fine with this new module at an overall level: #1503 |
modules/orientdb/src/test/java/org/testcontainers/containers/OrientDBGremlinTest.java
Outdated
Show resolved
Hide resolved
...orientdb/src/test/java/org/testcontainers/containers/OrientDBContainerWithClassRuleTest.java
Outdated
Show resolved
Hide resolved
removes class rule test add a gremlin test more generic updates to orientdb:3.0.22-tp3
@Test | ||
public void testContainerLifecycle() { | ||
OrientDBContainer container = new OrientDBContainer(); | ||
|
||
container.start(); | ||
|
||
assertThat(container.isRunning()).isTrue(); | ||
|
||
container.stop(); | ||
|
||
assertThat(container.isRunning()).isFalse(); | ||
} |
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 think this test is unnecessary, since it's a feature of Testcontainers core. Can we remove?
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.
removed
public OrientDBContainer withUsername(final String username) { | ||
this.username = username; | ||
return self(); | ||
} | ||
|
||
public OrientDBContainer withPassword(final String password) { | ||
this.password = password; | ||
return self(); | ||
} |
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 username and password don't seem to be used to configure the container. In the JDBC database classes, username/password are passed through to the container via environment variables, to tell the container's bootstrap script to create that account.
It doesn't look like OrientDB supports this natively. Line 147 might be an opportunity to create the database and give access to the provided username
, but as far as I can tell it's just expecting those credentials to work on line 150.
Is that accurate?
If these methods don't ensure that the account exists, I'd prefer to remove them to avoid confusion, TBH.
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.
You're right. I misunderstood the purpose of the member variables of the Container class, mixing server and client variables. I moved to the getSession method.
modules/orientdb/src/main/java/org/testcontainers/containers/OrientDBContainer.java
Show resolved
Hide resolved
private static final String DEFAULT_IMAGE_NAME = "orientdb"; | ||
|
||
private static final String DEFAULT_TAG = "3.0.24-tp3"; | ||
|
||
private static final String DEFAULT_USERNAME = "admin"; | ||
|
||
private static final String DEFAULT_PASSWORD = "admin"; | ||
|
||
private static final String DEFAULT_DATABASE_NAME = "testcontainers"; | ||
|
||
private static final int DEFAULT_BINARY_PORT = 2424; | ||
|
||
private static final int DEFAULT_HTTP_PORT = 2480; | ||
|
||
private static final Logger LOGGER = LoggerFactory.getLogger(OrientDBContainer.class); | ||
|
||
private static final String DOCKER_IMAGE_NAME = DEFAULT_IMAGE_NAME + ":" + DEFAULT_TAG; | ||
|
||
private String databaseName; | ||
|
||
private String username; | ||
|
||
private String password; | ||
|
||
private OrientDB orientDB; | ||
|
||
private ODatabaseSession session; | ||
|
||
private Optional<String> scriptPath = Optional.empty(); |
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.
Just a tiny style point, but I'd be inclined to reduce the spacing and reorder some of these fields (to group similar fields together, and to reduce visible space taken).
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.
cleaned
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.
Hi @robfrank, sorry for the epic amount of time it's taken for me to have a proper look at this. I've added some comments.
If you're still interested, and able to, continue this then it'd be great if you could. If you can't, please let us know all the same.
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.
@robfrank - thank you for the updates following review, and again I'm sorry that this has been open for so long. Will merge - thanks for the contribution!
Co-authored-by: Richard North <rich.north@gmail.com> Co-authored-by: Sergei Egorov <bsideup@gmail.com>
Released in 1.13.0! Thanks for the contribution @robfrank 😃 |
Co-authored-by: Richard North <rich.north@gmail.com> Co-authored-by: Sergei Egorov <bsideup@gmail.com>
This adds: