chore: made deriving Clone consistent
#156
Conversation
| @@ -8,7 +8,7 @@ use testcontainers::{ | |||
| const NAME: &str = "docker.elastic.co/elasticsearch/elasticsearch"; | |||
| const TAG: &str = "7.16.1"; | |||
|
|
|||
| #[derive(Debug, Default)] | |||
| #[derive(Debug, Default, Clone)] | |||
| pub struct ElasticSearch { | |||
| _priv: (), | |||
There was a problem hiding this comment.
One thing that I found interesting is this pattern vs (for example) pub struct DynamoDb;
Out of curiosity:
Is there a reason for having this unused member variable? ^^
There was a problem hiding this comment.
Is there a reason for having this unused member variable? ^^
Yes, to provide encapsulation, this allows the image to be extended without breaking future changes.
pub struct DynamoDb; allows you to create an instance simply by using DynamoDb (e.g DynamoDb.start()). Therefore adding a new field will break this usage.
Although ElasticSearch requires the use of new() or default() so that we can safely add new fields.
However, some images are still missing this pattern and should be updated in the future
There was a problem hiding this comment.
Thank you very much!
I'd consider this as a feature, we introduce new allowed functionality in general.
(I don't have a real usecase for this. Just thought that more consistency can't hurt)
I think one possible use-case is to spawn several pre-configured instances, something like:
let image = MssqlServer::default().with_sa_password("my_password");
let container_1 = image.clone().start();
let container_2 = image.start()It's unlikely to be common pattern, but there is no reason to restrict users of doing this.
|
Forgot to change Merged, thanks! |
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()` |
This is a followup to #154 (comment)
(I don't have a real usecase for this. Just thought that more consistency can't hurt)
Neo4jImageis ommited, asContainerStatedoes not implementClone.