-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Upgrade Neo4j server and driver dependencies. #4789
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.
LGTM, WDYT about removing Neo4jTestImages
?
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java
Outdated
Show resolved
Hide resolved
modules/neo4j/src/test/java/org/testcontainers/containers/Neo4jTestImages.java
Show resolved
Hide resolved
@@ -250,4 +257,9 @@ private static String formatConfigurationKey(String plainConfigKey) { | |||
.replaceAll("_", "__") | |||
.replaceAll("\\.", "_")); | |||
} | |||
|
|||
private boolean usesVersion(String version) { |
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.
We're a bit cautious that simply looking at the start of the version/tag could be a bit fragile in the case of custom images that people may be using, or simply surprising version numbers from any source.
Please could you have a look at how a similar problem is handled in LocalStackContainer
?
https://github.com/testcontainers/testcontainers-java/blob/master/modules/localstack/src/main/java/org/testcontainers/containers/localstack/LocalStackContainer.java#L100
There are three main facets which (I hope!) add up to a safer implementation:
- use our ComparableVersion utility class to do a semver comparison, not startsWith
- gracefully fall back if a custom or non-parsable version is found - clearly logging what’s happened and how to change it
- provide a constructor to allow the automatic version detection to be overridden, if a custom version string is being used that clashes with the original
It's not perfect - edge cases can still exist - but this seems to be working well for users.
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 feedback. I changed the code to use ComparableVersion
(hopefully right).
I refrained from the additional constructor because the feature can only used with older versions of Neo4j already today.
I do not want to have
a) an additional constructor added to the mix of the already available ones. This could confuse the users.
b) encourage users to use an older version for convenience
Both might even result in users trying to somehow hack their way around the restriction and end up with a broken database inside the container.
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.
@meistermeier thanks for switching to using ComparableVersion
and for explaining your intentions around the constructors. I think this is fine, on balance, and am happy to approve.
This PR upgrades
Two things that I would like to improve but do not know how to solve them:
Having to explicitly say that the database from the test-resources needs to get upgraded every time. @michael-simons https://github.com/testcontainers/testcontainers-java/compare/master...meistermeier:upgrade-neo4j?expand=1#diff-7f1bb307bc0ede77f2ef4beb5966f2187e591768eabeb9733b4796df6a689137R50