testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.82k stars 278 forks source link

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

Closed Sossenbinder closed 1 month ago

Sossenbinder commented 2 months ago

What does this PR do?

See https://github.com/testcontainers/testcontainers-dotnet/issues/1261

Why is it important?

Purely QoL, there are no immediate functional benefits.

Related issues

netlify[bot] commented 2 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit c80e83007db83fb51b53668fe5983225c24aa175
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.

Sossenbinder commented 1 month ago

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 ๐Ÿ˜„

Sossenbinder commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

HofmeisterAn commented 1 month ago

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 commented 1 month ago

Sure, go ahead, that works for me ๐Ÿ‘Thanks