-
Notifications
You must be signed in to change notification settings - Fork 45
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 mongodb replset option with example #143
Conversation
e61b2a6
to
01898b7
Compare
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.
Thank you for the contribution! 🙏 I really appreciate it!
I left couple of comments, please take a look and let me know what do you think.
Also, since we're touching the module, it would be great to add some basic documentation, for example like this one:
testcontainers-rs-modules-community/src/rabbitmq/mod.rs
Lines 6 to 27 in edf3c6e
/// Module to work with [`RabbitMQ`] inside of tests. | |
/// | |
/// Starts an instance of RabbitMQ with the [`management-plugin`] started by default, | |
/// so you are able to use the [`RabbitMQ Management HTTP API`] to manage the configuration if the started [`RabbitMQ`] instance at test runtime. | |
/// | |
/// This module is based on the official [`RabbitMQ docker image`]. | |
/// | |
/// # Example | |
/// ``` | |
/// use testcontainers_modules::{rabbitmq, testcontainers::runners::SyncRunner}; | |
/// | |
/// let rabbitmq_instance = rabbitmq::RabbitMq.start().unwrap(); | |
/// | |
/// let amqp_url = format!("amqp://{}:{}", rabbitmq_instance.get_host().unwrap(), rabbitmq_instance.get_host_port_ipv4(5672).unwrap()); | |
/// | |
/// // do something with the started rabbitmq instance.. | |
/// ``` | |
/// | |
/// [`RabbitMQ`]: https://www.rabbitmq.com/ | |
/// [`management-plugin`]: https://www.rabbitmq.com/management.html | |
/// [`RabbitMQ Management HTTP API`]: https://www.rabbitmq.com/management.html#http-api | |
/// [`RabbitMQ docker image`]: https://hub.docker.com/_/rabbitmq |
@DDtKey
I totally agree with you that we should expose only a single Mongo image for the user's convenience. As you mentioned, the representation of arguments makes it difficult to run different arguments driven by different options. Refactoring will be really helpful! I think I may refactor my code after the release of testcontainers that includes the PR. |
And I would like to add examples for mongodb |
- really common pattern is to use `Vec<String>` - `args` isn't the best term, because in fact it's Docker's [CMD](https://docs.docker.com/reference/dockerfile/#cmd) - it was not aligned with existing `ExecCommand` - current interface is confusing and users struggle with it - essentially it should be closer to the current representation of environment variables and other parameters - new implementation allows to override args regardless of underlying type - it's still will be possible to use custom strict arguments type for particular image which can be converted to list of strings if users want Dependent PR to modules: testcontainers/testcontainers-rs-modules-community#143
…#649) - really common pattern is to use `Vec<String>` - `args` isn't the best term, because in fact it's Docker's [CMD](https://docs.docker.com/reference/dockerfile/#cmd) - it was not aligned with existing `ExecCommand` - current interface is confusing and users struggle with it - essentially it should be closer to the current representation of environment variables and other parameters - new implementation allows to override args regardless of underlying type - it's still will be possible to use custom strict arguments type for particular image which can be converted to list of strings if users want Dependent PR to modules: testcontainers/testcontainers-rs-modules-community#143 Also, I'm preparing 2 follow-up PRs which will improve the user experience.
Hi @sinyo-matu 👋 Sorry for delay, totally forgot to notify you that a new version of testcontainers has been released. Now it's possible to encapsulate arguments as part of image itself! |
Sorry for the delay!
|
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.
Thank you!
Just one question, but in general looks like LGTM
src/mongo/mod.rs
Outdated
ExecCommand::new(vec![ | ||
"mongosh".to_string(), | ||
"--eval".to_string(), | ||
"'rs.status()'".to_string(), | ||
]) | ||
.with_cmd_ready_condition(CmdWaitFor::message_on_stdout("ok: 1")), |
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.
I think rs.status
doesn't wait for "ok", right?
In that case we probably need to wrap it into something like:
until mongosh --eval 'rs.status()'; do sleep 1; done
WDYT?
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.
However, I guess the command will return 0 exit status even for status!= ok 🤔
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.
Yes you're right. that is tricky🤔.
Another thought:
I confirmed below log outputs to stdout at the last parts of the replSet starting process.
So we can wait for it by with_container_ready_conditions
Rebuilding PrimaryOnlyService due to stepUp
And it is working for tests and my application usage.
There are still few starting processes after this log... so it is working for general situation IMO, but not a so solid solution.
Updated my code.
How about it?
@DDtKey |
## 🤖 New release * `testcontainers-modules`: 0.8.0 -> 0.9.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.9.0] - 2024-07-30 ### Features - Expose `watchdog` feature of testcontainers ([#168](#168)) - [**breaking**] Add mongodb replset option with example ([#143](#143)) - [**breaking**] Update testcontainers to 0.21.0 ([#172](#172)) ### Miscellaneous Tasks - Update redis requirement from 0.25.0 to 0.26.0 ([#171](#171)) <!-- generated by git-cliff --> </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR makes sure that the way structs are constructed is consistent and encourages the builder pattern. It is based on this question: #156 (comment) For the changelog: We now require you to use the builder pattern (instead of some structs being [unit structs](https://doc.rust-lang.org/std/keyword.struct.html)) for all modules. This ensures that if we add a field in the future that this will not break your existing code. This change is breaking for these modules: | Module | before | after | |--------|--------|--------| | `cncf_distribution::CncfDistribution` | `CncfDistribution.start()` | `CncfDistribution::default().start()` | | `dynamodb_local::DynamoDb` | `DynamoDb.start()` | `DynamoDb::default().start()` | | `elasticmq::ElasticMq` | `ElasticMq.start()` | `ElasticMq::default().start()` | | ~`mongo::Mongo`~ (see #143) | ~`Mongo.start()`~ | ~`Mongo::default().start()`~ | | `kwok::KwokCluster` | `KwokCluster.start()` | `KwokCluster::default().start()` | | `rabbitmq::RabbitMq` | `RabbitMq.start()` | `RabbitMq::default().start()` | | `redis::stack::RedisStack` | `RedisStack.start()` | `RedisStack::default().start()` | | `redis::standalone::Redis` | `Redis.start()` | `Redis::default().start()` | | `victoria_metrics::VictoriaMetrics` | `VictoriaMetrics.start()` | `VictoriaMetrics::default().start()` |
Thank you for the amazing work.
I really enjoy using testcontainers with Rust.
When working with MongoDB, it is very common to need to test transaction operations. Therefore, I added some options to facilitate this.
Mongo.start()
will no longer work. Instead, users need to explicitly callMongo::default()
.