Deprecate withEnterpriseEdition due to version pinning #10613
Conversation
| } | ||
|
|
||
| setDockerImageName(DEFAULT_IMAGE_NAME.withTag(ENTERPRISE_TAG).asCanonicalNameString()); | ||
| setDockerImageName(DockerImageName.parse(getDockerImageName() + ENTERPRISE_SUFFIX).asCanonicalNameString()); |
There was a problem hiding this comment.
this will break existing users. Why not improve the exception's message?
There was a problem hiding this comment.
From a technical, formal perspective I would say, that you are right.
But (of course) I would consider this as a bug that came as a left-over when we've removed the default constructor that fell back to the default image name.
I think that nobody would expect working with a neo4j:5.26 image to get ported back to the previous major version (4.4) when enabling enterprise per API.
There was a problem hiding this comment.
a) how many users are on 4.4 still And expect the enterprise version of it when they actually want 5.26 or basically anything else, it's a bug
b) If we omit one hardcoded version (like it it seems with the default image), that one has to go for consistency.
There was a problem hiding this comment.
I would like to propose something else:
Be explicit in breaking the current behavior (by removing withEnterpriseEdition() in its current form, which indeed is a crazy confusing method implementation), and maybe have a new method, that gets the version (tag?) as an argument.
How does that sound?
There was a problem hiding this comment.
Will play with this. Thanks for the feedback.
This will not only update the test image but also clean up usage of `withEnterpriseEdition` to just append the `-enterprise` suffix instead of pulling in the former neo4j:4.4 default image without users noting the implicit change from their defined image version.
meistermeier
force-pushed
the
neo4j-lts-5
branch
from
f090ec8 to
32ee567
Compare
|
Updated the PR do work with main after the JUnit upgrade. |
|
@meistermeier I'm probably missing something since I was out of the game for some time, but isn't the only thing this PR effectively does now, is bumping the image in the tests (without effective changes to the module behavior besides an internal refactoring)? Or asked differently, which usage was failing before, that would work now? (which we could add as a new test accordingly) |
|
You're pretty much spot-on now... The test (and minor renaming) is all that's left. But it's an improvement for the test suite nevertheless because it targets (mostly) everywhere the latest LTS. But for now it's the only thing I can add if the breaking API with a real improvement does not get accepted. |
meistermeier
changed the title
|
@kiview the image in tests was bumped to unsure a consistent behaviour for both the community and enterprise edition. |
Deprecate the existing withEnterpriseEdition method and adjust the documentation accordingly.
|
Naming is so hard, so I came up with
|
meistermeier
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR deprecates the withEnterpriseEdition() method in favor of a new withEnterpriseImage() method to address version pinning issues. The deprecated method was implicitly changing user-specified Neo4j image versions to 4.4-enterprise, which could surprise users who expected their chosen version to be preserved.
Key changes:
- Added a version-agnostic
withEnterpriseImage()method that appends "-enterprise" to the user's specified image version - Deprecated
withEnterpriseEdition()method with appropriate warnings about its version-forcing behavior - Updated test suite to use Neo4j 5.26 (latest LTS) instead of the previous 4.4 version
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Neo4jContainer.java | Added new withEnterpriseImage() method and deprecated withEnterpriseEdition() with version pinning fixes |
| Neo4jContainerTest.java | Updated test cases to use Neo4j 5.26 and the new withEnterpriseImage() method |
| neo4j.md | Updated documentation to reflect the deprecation and recommend the new method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java
Show resolved
Hide resolved
modules/neo4j/src/main/java/org/testcontainers/containers/Neo4jContainer.java
Outdated
Show resolved
Hide resolved
modules/neo4j/src/test/java/org/testcontainers/containers/Neo4jContainerTest.java
Outdated
Show resolved
Hide resolved
kiview
left a comment
kiview
left a comment
There was a problem hiding this comment.
@meistermeier I think that is a nice hack to deprecate the problematic old method and add the new desired behavior, without ~breaking existing users.
LGTM, any objections @eddumelendez ?
The versions need to be set explicitly to avoid the wrong version for testing when using the test constant.
|
Thanks for the review @kiview, PR updated. |
4 participants
Deprecate the method in favour of a version-agnostic replacement. The
withEnterpriseEditionpulls in the former neo4j:4.4 default image / LTS version without users noting the implicit change from their defined image version.A
Neo4jContainer("neo4j:5.26").withEnterpriseEdition()becomesneo4j:4.4-enterpriseand let's say that this is at least a surprise, even though it's explicitly mentioned in the documentation.Also upgrade the test images to be used to 5.26 (latest LTS version) where possible.