feat: add etcd module #2788
feat: add etcd module #2788
Conversation
mdelapenya
added
the
feature
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* main: feat: support databend module (testcontainers#2779) chore: golangci-lint 1.61.0 (testcontainers#2787)
* main: chore: use a much smaller image for testing (testcontainers#2795) fix: parallel containers clean race (testcontainers#2790) fix(registry): wait for (testcontainers#2793) fix: container timeout test (testcontainers#2792) docs: document redpanda options (testcontainers#2789)
modules/etcd/etcd.go
Outdated
| func configureCMD(settings options) []string { | ||
| cmds := []string{"etcd"} | ||
|
|
||
| if len(settings.nodeNames) < 1 { |
There was a problem hiding this comment.
question: do we want to allow a cluster of 1 node, doesn't really make sense?
There was a problem hiding this comment.
I think it could be the case you want a single etcd instance
There was a problem hiding this comment.
Single instance yes but not single node cluster?
There was a problem hiding this comment.
I don't know the semantical difference between being a one-node cluster Vs being one instance (e.g. one elasticsearch instance is the same as one-node elasticsearch cluster).
In our case, the code just detects there is one node/instance, and it returns a single container. When the user requests a cluster (more than one nodes), then we return a parent container with children. So I do not see the difference here.
I could picture the Run function returning just one container and an eventual RunCluster one that separates concerns. But this is the purpose of the WithNodes option, right?
There was a problem hiding this comment.
Yer I don't see the need for a RunCluster, but what I saw was all the additional command line options complexity for cluster setup which could be avoided for a single node. I believe the only reason etcd supports a single node cluster is so you can add nodes at a later date, which doesn't seem like a scenario you would need for testing.
There was a problem hiding this comment.
I believe the only reason etcd supports a single node cluster is so you can add nodes at a later date
Indeed. When I started the feature it was to support two scenarios:
- one etcd node
- multiple etcd nodes in a cluster
No more. Wdyt if we start like this, and check with the community how it's used. For us, deprecating the option would be enough for an eventual removal of the cluster support.
There was a problem hiding this comment.
You raise a good point, do we even need cluster support, should we just keep a patch about and add it later if there is demand, that avoids the hassle of deprecation?
It would also removed the need for ClientEndpoints and PeerEndpoints too by the looks
There was a problem hiding this comment.
Let me add a removal commit that allows us to go back to it later.
I recalled that I ported the Java implementation. Should we still start simple?
There was a problem hiding this comment.
I think so, I might be wrong. I guess someone might want to test their app worked correctly if they killed a cluster node, but that would be an edge case and need additional work to expose the nodes themselves.
There was a problem hiding this comment.
@eddumelendez @kiview what are your thoughts about feature parity across modules in the libraries? Should we start simple with no cluster support, or just keep what this PR does?
* main: fix(elasticsearch): wait for (testcontainers#2724)
This reverts commit 1c4361b.
* main: feat: add dynamodb-local module (testcontainers#2799) fix(redpanda): wait for (testcontainers#2794)
What does this PR do?
This PR adds a Go module for etcd, including clustering support. This module includes options to configure:
auto TLS for cluster-node communicationI removed it because it was not critical and causing issues in testsI followed https://etcd.io/docs/v3.5/op-guide/clustering/ to accomplish it.
The module exposes methods to talk to the the client and the peer endpoints.
Why is it important?
A new module for etcd, which could unblock adoption in other technologies (gofiber's etcd storage)