Skip to content
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

feat: Add Neo4j Enterprise Edition support (WithEnterpriseEdition(bool)) #1269

Conversation

Sossenbinder
Copy link
Contributor

What does this PR do?

See #1261

  • Added small QoL method to support Neo4j enterprise
  • Added validation to throw when enterprise is requested without the correct license agreement environment variable
  • Added tests for enterprise & configuration changes
  • Changed existing test to use IAsyncDisposable instead of sync-over-async disposal

Why is it important?

Purely QoL, there are no immediate functional benefits.

Related issues

Copy link

netlify bot commented Sep 22, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit c80e830
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/66fecbe26f73b000091e107a
😎 Deploy Preview https://deploy-preview-1269--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- Added small QoL method to support Neo4j enterprise
- Added validation to throw when enterprise is requested without the correct license agreement environment variable
- Added tests for enterprise & configuration changes
- Changed existing test to use IAsyncDisposable instead of sync-over-async disposal
@Sossenbinder Sossenbinder force-pushed the GH-1261_support_neo4j_enterprise branch from 5f26897 to 1797277 Compare September 22, 2024 14:25
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

I made some minor adjustments and organized the using statements. Additionally, I changed the member to WithEnterpriseEdition(bool). I still expect developers to agree to the license, as we cannot do that for them. I would like to add the new member to the documentation as well; then I am happy to merge it. LMKWYT. Thanks for the PR!

@Sossenbinder
Copy link
Contributor Author

I made some minor adjustments and organized the using statements. Additionally, I changed the member to WithEnterpriseEdition(bool). I still expect developers to agree to the license, as we cannot do that for them. I would like to add the new member to the documentation as well; then I am happy to merge it. LMKWYT. Thanks for the PR!

Sounds perfectly fine to me 👍 I'm happy to adhere to the repo standards modified from your side, so this works for me.

Thanks for applying them, this is probably something I missed while working on the very niche code section required for my changes 😄

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

There are two small things I would like to mention:

  • Java uses a slightly different approach to handle license agreements for images. Specifically, they use LicenseAcceptance, which reads accepted license agreements for images from a file. Perhaps we can consider adopting a similar mechanism in the future to support the same approach across all modules.

  • The current implementation has a minor flaw. If someone wants to use a different version of Neo4j (pin a version), they need to use the builder methods in the correct order. This will fall back to the default image version (not something we want):

    _ = new Neo4jBuilder().WithImage("neo4j:5.23-enterprise").WithEnterpriseEdition(true).Build();

    I haven’t looked into the Java implementation, so I’m not sure if they encounter a similar issue. My initial thought is that we could change the image version only if it’s not already an enterprise version. Additionally, it might make sense to just append the -enterprise tag?

@Sossenbinder
Copy link
Contributor Author

There are two small things I would like to mention:

  • Java uses a slightly different approach to handle license agreements for images. Specifically, they use LicenseAcceptance, which reads accepted license agreements for images from a file. Perhaps we can consider adopting a similar mechanism in the future to support the same approach across all modules.

  • The current implementation has a minor flaw. If someone wants to use a different version of Neo4j (pin a version), they need to use the builder methods in the correct order. This will fall back to the default image version (not something we want):

    _ = new Neo4jBuilder().WithImage("neo4j:5.23-enterprise").WithEnterpriseEdition(true).Build();

    I haven’t looked into the Java implementation, so I’m not sure if they encounter a similar issue. My initial thought is that we could change the image version only if it’s not already an enterprise version. Additionally, it might make sense to just append the -enterprise tag?

That's a fair point. I saw the Java implementation as well, but kind of skipped the file based agreement for now. It feels like that's more of a Java ecosystem style, at least it felt pretty unfamiliar from other nuget packages I used in the dotnet space, so I would agree we might want to add this for the sake of consistency. But probably not an immediate requirement.

Ah, I agree on the second point. I'm on vacation right now, so I can take this over once I'm back next week 👍

@HofmeisterAn
Copy link
Collaborator

Ah, I agree on the second point. I'm on vacation right now, so I can take this over once I'm back next week 👍

No rush. I might find some time to add it in the meantime. Have a nice vacation 🌞.

@Sossenbinder
Copy link
Contributor Author

Nice work! That seems pretty extensive now, I think this should do it for all cases.

@HofmeisterAn
Copy link
Collaborator

Nice work! That seems pretty extensive now, I think this should do it for all cases.

I had some time this afternoon to think about it, but I bet there are still a few edge cases that aren’t covered. However, it should definitely work for most cases, though. If you're okay with it, I’m happy to go ahead and merge it. Thanks for the PR!

@Sossenbinder
Copy link
Contributor Author

Sure, go ahead, that works for me 👍Thanks

@HofmeisterAn HofmeisterAn changed the title GH-1261 [Enhancement]: Support Neo4j Enterprise feat: Add Neo4j Enterprise Edition support (WithEnterpriseEdition(bool)) Oct 3, 2024
@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Oct 3, 2024
@HofmeisterAn HofmeisterAn merged commit e96d4e6 into testcontainers:develop Oct 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Support Neo4J Enterprise
2 participants