Thanks for looking into improvements for the tests!

I think this is a good approach 👍 . The alternative would be to have another switch statement and then depending on the store do some request with curl, but that wouldn't work with stores that require a container-internal CLI command, or expose a health endpoint on a port that we don't need for regular store access. Also, some images might already define a healthcheck themselves (with this) and executing the same check manually would feel redundant.

This way we can check docker inspect --format='{{.State.Health.Status}}' in a loop with sleep and be done with it, 👍

Have you checked already which of the stores need a check passed in the run command, and which provide one already in the image? Depending on that maybe we can do all at once in a single PR?

Have you checked already which of the stores need a check passed in the run command, and which provide one already in the image? Depending on that maybe we can do all at once in a single PR?

none of those have healcheck:

docker run -d --rm --name cockroachdb -p 26257:26257 cockroachdb/cockroach start-single-node --insecure
docker run -d --rm --name consul -e CONSUL_LOCAL_CONFIG='{"limits":{"http_max_conns_per_client":1000}}' -p 8500:8500 bitnami/consul
docker run -d --rm --name datastore -p 8081:8081 google/cloud-sdk gcloud beta emulators datastore start --no-store-on-disk --project=gokv --host-port=0.0.0.0:8081
docker run -d --rm --name dynamodb-local -p 8000:8000 amazon/dynamodb-local
docker run -d --rm --name etcd -p 2379:2379 --env ALLOW_NONE_AUTHENTICATION=yes bitnami/etcd
docker run -d --rm --name hazelcast -p 5701:5701 hazelcast/hazelcast
docker run -d --rm --name ignite -p 10800:10800 apacheignite/ignite
docker run -d --rm --name memcached -p 11211:11211 memcached
docker run -d --rm --name mongodb -p 27017:27017 mongo
docker run -d --rm --name mysql -e MYSQL_ALLOW_EMPTY_PASSWORD=true -p 3306:3306 mysql
docker run -d --rm --name postgres -e POSTGRES_PASSWORD=secret -e POSTGRES_DB=gokv -p 5432:5432 postgres:alpine
docker run -d --rm --name redis -p 6379:6379 redis
docker run -d --rm --name s3 -e "MINIO_ACCESS_KEY=AKIAIOSFODNN7EXAMPLE" -e "MINIO_SECRET_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" -p 9000:9000 minio/minio server /data
docker run -d --rm --name zookeeper -p 2181:2181 zookeeper

This way we can check docker inspect --format='{{.State.Health.Status}}' in a loop with sleep and be done with it, 👍

yes, that is the idea, exactly. just need to add healthchecks to all the above

Codecov Report

Merging #142 (ccf85f5) into master (087cefb) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #142   +/-   ##
=======================================
  Coverage   65.26%   65.26%           
=======================================
  Files          25       25           
  Lines        2096     2096           
=======================================
  Hits         1368     1368           
  Misses        601      601           
  Partials      127      127           

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

left to fix 4 dockers:

$ git grep dockerCmd magefiles/ | grep -v health | grep run
magefiles/test.go:              dockerCmd = `docker run -d --rm --name consul -e CONSUL_LOCAL_CONFIG='{"limits":{"http_max_conns_per_client":1000}}' -p 8500:8500 bitnami/consul`
magefiles/test.go:              dockerCmd = `docker run -d --rm --name datastore -p 8081:8081 google/cloud-sdk gcloud beta emulators datastore start --no-store-on-disk --project=gokv --host-port=0.0.0.0:8081`
magefiles/test.go:              dockerCmd = `docker run -d --rm --name dynamodb-local -p 8000:8000 amazon/dynamodb-local`
magefiles/test.go:              dockerCmd = `docker run -d --rm --name memcached -p 11211:11211 memcached`

current problems:

[dynamodblocal@46bc3291633d ~]$ curl http://localhost:8000/shell/
{"__type":"com.amazonaws.dynamodb.v20120810#MissingAuthenticationToken","Message":"Request must contain either a valid (registered) AWS access key ID or X.509 certificate."}

consul@8dfcfd78cc76:/$ curl -f http://localhost:5000/health
bash: curl: command not found

memcache@344f2239c5ba:/$ nc -z 127.0.0.1 11211
bash: nc: command not found

@philippgille I think is is ready to merge

Thanks for adding so many health checks already! I think we can add some more:

• Consul: curl http://localhost:8500/v1/health/state/passing | grep serfHealth

• Memcached is a bit more complex:

  • dockerCmd = `docker run -d --rm --name memcached -p 11211:11211 -u root --health-cmd='echo stats | nc -w 1 localhost 11211' memcached bash -c 'apt update && apt install -y netcat && docker-entrypoint.sh memcached -u memcache'`

And then we can also replace the time.Sleep(10 * time.Second) to loop over the docker inspect ..., right? And we can detect the containers that don't have a healthcheck by looking at the inspect output, which is empty (instead of starting/healthy) I guess, and then we do a single 10s sleep, or so?

1. bitnami/consul doesn;t have curl
2. i don't like running apt install during runtime...
3. agree once we have healthchecks, removing sleep and waiting for health ready status

do you think the existing PR is mergable or you want to resolve those 3 above first ?