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 image #66

Merged
merged 11 commits into from
Nov 23, 2023
Merged

feat: add Neo4j image #66

merged 11 commits into from
Nov 23, 2023

Conversation

knutwalker
Copy link
Contributor

@knutwalker knutwalker commented Nov 14, 2023

No description provided.

@mervyn-mccreight
Copy link
Contributor

First of all thanks for the contribution!

To fix the failing PR title check you need to change the subject to start with lower-case (Add neo4j image vs. add neo4j image) I think.

@knutwalker knutwalker changed the title feat: Add Neo4j image feat: add Neo4j image Nov 15, 2023
@knutwalker
Copy link
Contributor Author

@mervyn-mccreight thanks! I updated the PR title, it should work now according to https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional#subject-case. Can you run the check again in case it's not picked up by Github?

Copy link
Contributor

@mervyn-mccreight mervyn-mccreight left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I had a look and primarily have left some questions that came to my mind.

src/neo4j/mod.rs Outdated Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

First of all, thank you for the contribution! 🚀

It looks pretty nice and convenient to use, but I have couple of concerns:

  • Ability to inspect user's ENV vars & files is questionable.
    • any change of these variables would require major update (breaking change)
    • even if we want to have from_env, we need to think how to generalize this over all images, instead of implementing this to particular one (btw, could be a good outcome)
    • we should ensure that environment variables used only for from_env and it's consistent over time
  • license check is also seems pretty hardcoded & depends on environment. See related comment

But mostly it's just topics to discuss, there seems to be no need for much effort to get it merged, just a cleanup

src/neo4j/mod.rs Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
src/neo4j/mod.rs Outdated Show resolved Hide resolved
@knutwalker
Copy link
Contributor Author

knutwalker commented Nov 22, 2023

Thanks for the review and the comments!

  • even if we want to have from_env, we need to think how to generalize this over all images, instead of implementing this to particular one (btw, could be a good outcome)
  • we should ensure that environment variables used only for from_env and it's consistent over time

I did start building this testcontainers module for the development of neo4j-labs/neo4rs where I wanted to add integration tests. I didn't really draw a good line between things that are relevant for testing neo4rs in particular and what should generally be available on the testcontainer and I'd say all the env vars and from_env is part of that.

Perhaps a generalized env handling is a good idea, but then not introduced by this PR.

I removed all the env business and one this PR is merged and the module released, I can switch neo4rs over to use this and move all the env stuff to the neo4rs repo.

I also addressed all the other inline comments and rebased onto the latest main.

Looking forward to the next round 🏓

src/neo4j/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all efforts!

Some notes after the PR:

  • would be nice to have some reusable utility to accept license
  • consider adding generalized from_env methods to our images

I'll prepare issues for these ones to cover & discuss

@DDtKey DDtKey merged commit c049d14 into testcontainers:main Nov 23, 2023
6 checks passed
@github-actions github-actions bot mentioned this pull request Nov 23, 2023
@DDtKey
Copy link
Contributor

DDtKey commented Nov 23, 2023

@knutwalker new version has been released: https://github.com/testcontainers/testcontainers-rs-modules-community/releases/tag/v0.1.4

You can use it now from crates.io!

@knutwalker
Copy link
Contributor Author

@DDtKey Thanks for merging and releasing! I will update knutwalker/neo4j-testcontainers-rs to re-export this module and declare it EOL and will move the ENV and license handling to the neo4rs test setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants