feat(modules.mongodb): setup mongodb replicaset#2139
feat(modules.mongodb): setup mongodb replicaset#2139trungdlp-wolffun wants to merge 2 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks for your contribution, @trungdlp-wolffun. Currently, the module support standalone mode by default. Adding ReplicaSet is really nice, however, IMO, this should be an opt-in feature. In the future, sharding can be added as well as a opt-in feature too. Please, do not forget to add proper test. |
|
|
||
| func replicaSetLifecycleHooks() testcontainers.ContainerLifecycleHooks { | ||
| return testcontainers.ContainerLifecycleHooks{ | ||
| PostStarts: []testcontainers.ContainerHook{ |
There was a problem hiding this comment.
I think it would be clearer if we append the container hook functions directly to the PostStarts hook, instead of appending a complete lifecycle hook struct. In the end, the result is the same, but having them all together in the same lifecycle hook seems better aligned, as they all belong to the same hook's post-start events. Wdyt?
There was a problem hiding this comment.
I move replicaSetPostStart code into replicaSetLifecycleHooks, right? @mdelapenya
There was a problem hiding this comment.
You could leverage the Options pattern we are seen in other modules, such as Redpanda, where the Options struct holds all the values to be applied and all the functional options to configure the module are setting the values into that options struct.
Finally, I'd define a container lifecycle hook in that options, and with each call to the WithReplicaSet function, I'd be appending to the PostStarts field of the lifecycle hook. Last thing would be to append the mongo lifecycle hook to the container request before starting the container, right after all the options have been applied.
Does it make sense to you?
There was a problem hiding this comment.
@trungdlp-wolffun please let us know if you need anything from us 🙏
| // WithReplicaSet TODO: help me fill this func comment | ||
| func WithReplicaSet(rsName string) testcontainers.CustomizeRequestOption { | ||
| return func(req *testcontainers.GenericContainerRequest) { | ||
| req.Env["MONGO_REPLICASET_NAME"] = rsName |
There was a problem hiding this comment.
why is setting the env var also required?
| mongodb.WithReplicaSet("rs0"), | ||
| testcontainers.WithWaitStrategy(wait.ForLog("Waiting for connections")), |
There was a problem hiding this comment.
IMO, the replicaSet opt-in should be just WithReplicaSet() and it should be stored in a flag used to set the right wait strategy and exec the commands in the container lifecycle. However, not sure how important is setting the replica set name but having a default could be just enough?
eddumelendez
left a comment
eddumelendez
left a comment
There was a problem hiding this comment.
Thanks for the update and sorry for the delay. Just left a few more comments to improve experience.
|
|
||
| if c.username != "" && c.password != "" { | ||
| connStr.WriteString(fmt.Sprintf("%s:%s@", c.username, c.password)) | ||
| return fmt.Sprintf("mongodb://%s:%s@%s:%s", c.username, c.password, host, port.Port()), nil |
There was a problem hiding this comment.
IIUC from this block, if there is an username and password, the connection string won't have anything about the replicaSet, even though it was passed as an option, right?
If not sure, you can extract the url-builder part to a function and add unit tests for it
|
Hi @trungdlp-wolffun I think #2469 has better readability so I think we are going to build on top of that. This PR will be closed once that one is merged. I'm sorry if the review took so long 🙏 Thanks! |
3 participants
What does this PR do?
Fix issue #2138
Why is it important?
Related issues