One of the tests is using fixed ports due to the compose definition. This commit move the compose file to Testcontainers in order to reduce the configuration around those containers and rely on best practices when testing those.
Comment From: tzolov
Hi @eddumelendez, thank you for the contributions.
I don't see, though, the benefit of re-implementing in Java a well encapsulated DSL such as docker-compose ?
Our tests are not interested in neither the etcd
nor the minio
services/containers. Why should we create and maintain explicit instances of those?
Also, I like to (re)use the docker-compose for manual experimenting or testing. I don't want to write java code that emulates docker compose achieve this.
Can you, please, elaborate on this:
One of the tests is using fixed ports due to the compose definition. and let see if/how we can mitigate it.
Comment From: eddumelendez
Hi @tzolov,
I don't see, though, the benefit of re-implementing in Java a well encapsulated DSL such as docker-compose ?
Besides reusing the code in different modules I would say it will help avoiding the issue described with the fixed port that I described in the last answer.
Our tests are not interested in neither the etcd nor the minio services/containers. Why should we create and maintain explicit instances of those?
What I have in mind is that the duplicated code I selected can be moved to an utility project where spring-ai-spring-boot-autoconfigure
and vector-stores/spring-ai-milvus-store
can use the MilvusContainer
without knowing about the details (etcd and minio) because as you said there is no interest on those during testing, those are just needed to start Milvus.
Example:
public class MilvusContainer extends GenericContainer<MilvusContainer> {
private static final Network network = Network.newNetwork();
private static final MinIOContainer minio = new MinIOContainer("minio/minio:RELEASE.2023-11-11T08-14-41Z")
.withNetwork(network)
.withNetworkAliases("minio");
private static final GenericContainer<?> etcd = new GenericContainer<>("quay.io/coreos/etcd:v3.5.5")
.withNetwork(network)
.withNetworkAliases("etcd")
.withCommand("etcd", "-advertise-client-urls=http://127.0.0.1:2379", "-listen-client-urls=http://0.0.0.0:2379",
"--data-dir=/etcd")
.withEnv(Map.of("ETCD_AUTO_COMPACTION_MODE", "revision", "ETCD_AUTO_COMPACTION_RETENTION", "1000",
"ETCD_QUOTA_BACKEND_BYTES", "4294967296", "ETCD_SNAPSHOT_COUNT", "50000"))
.waitingFor(Wait.forLogMessage(".*ready to serve client requests.*", 1));
public MilvusContainer(String image) {
dependsOn(etc, minio);
// more...
}
}
Also, I like to (re)use the docker-compose for manual experimenting or testing. I don't want to write java code that emulates docker compose achieve this.
This is totally fine 👍🏽
Can you, please, elaborate on this:
One of the tests is using fixed ports due to the compose definition.
This line set properties to create MilvusServiceClient
but instead of using the dynamic one is hardcoding localhost
and the fixed port. It could be fixed using milvusContainer.getServiceHost("standalone", 19530)
and milvusContainer.getServicePort("standalone", 19530)
. But, IMHO this is the kind of missing pieces when using Docker compose and indeed by doing through the GenericContainer the test would have failed immediately.
I totally get the part about experimenting and testing with Docker Compose and if it is the major reason to keep it, feel free to close the PR. My main motivation is to avoid pitfalls when using Docker Compose.
Comment From: tzolov
Hi @eddumelendez ,
I fixed the host/port resolution, thanks for pointing it out.
Regarding the docker-compose, currently it affects just Milvus so lets keep it as it is.
If situation change in the future we can review this again.
I appreciate your contributions! Keep them coming ;)