Copilot commented on code in PR #1039:
URL: https://github.com/apache/dubbo-go-samples/pull/1039#discussion_r2825532322
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
Review Comment:
The previous health check script explicitly validated the Nacos gRPC port
(9848) in addition to the HTTP endpoint. The current health check only
validates the HTTP endpoint. Since port 9849 has been added to the exposed
ports and Nacos 2.x uses gRPC for service communication, consider adding a
health check for the gRPC ports or using a compound health check command that
validates both HTTP and gRPC endpoints to ensure complete service readiness.
```suggestion
test: [ "CMD-SHELL", "curl -f http://localhost:8848/nacos/ && nc -z
localhost 9848 && nc -z localhost 9849" ]
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 30s
+ restart: unless-stopped
+ networks: [itnet]
polaris:
- image: polarismesh/polaris-standalone:latest
- container_name: polaris-standalone
+ image: polarismesh/polaris-standalone:v1.17.2
privileged: true
ports:
- "8090:8090"
- "8091:8091"
- "8093:8093"
healthcheck:
- test: "curl --fail http://127.0.0.1:8090 || exit 1"
+ test: [ "CMD", "curl", "-fsS", "http://localhost:8090/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 20s
+ restart: unless-stopped
+ networks: [itnet]
etcd:
- image: "quay.io/coreos/etcd:v3.4.28"
- container_name: etcd
+ image: quay.io/coreos/etcd:v3.4.28
environment:
- - ETCDCTL_API=3
- command: [
- "etcd",
- "--name=etcd0",
- "--advertise-client-urls=http://127.0.0.1:2379";,
- "--listen-client-urls=http://0.0.0.0:2379";,
- "--initial-advertise-peer-urls=http://127.0.0.1:2380";,
- "--listen-peer-urls=http://0.0.0.0:2380";,
- "--initial-cluster=etcd0=http://127.0.0.1:2380";,
- ]
+ ETCDCTL_API: "3"
+ command:
+ - etcd
+ - --name=etcd0
+ - --data-dir=/etcd-data
+ - --listen-client-urls=http://0.0.0.0:2379
+ - --advertise-client-urls=http://etcd:2379
Review Comment:
The advertise-client-urls has been changed from `http://127.0.0.1:2379` to
`http://etcd:2379`. This is problematic because the advertise URL is what etcd
tells clients to use for connecting. Since the etcd service is accessed from
the host machine via port mapping (port 2379 is exposed), and the test clients
connect using `127.0.0.1:2379` (as seen in
registry/etcd/go-client/cmd/client.go and
registry/etcd/go-server/cmd/server.go), changing the advertise URL to the
Docker internal hostname `etcd` will break client connectivity. The
advertise-client-urls should remain as `http://127.0.0.1:2379` or
`http://0.0.0.0:2379` to work with external clients accessing via the port
mapping.
```suggestion
- --advertise-client-urls=http://127.0.0.1:2379
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 30s
+ restart: unless-stopped
+ networks: [itnet]
polaris:
- image: polarismesh/polaris-standalone:latest
- container_name: polaris-standalone
+ image: polarismesh/polaris-standalone:v1.17.2
privileged: true
ports:
- "8090:8090"
- "8091:8091"
- "8093:8093"
healthcheck:
- test: "curl --fail http://127.0.0.1:8090 || exit 1"
+ test: [ "CMD", "curl", "-fsS", "http://localhost:8090/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 20s
+ restart: unless-stopped
+ networks: [itnet]
etcd:
- image: "quay.io/coreos/etcd:v3.4.28"
- container_name: etcd
+ image: quay.io/coreos/etcd:v3.4.28
environment:
- - ETCDCTL_API=3
- command: [
- "etcd",
- "--name=etcd0",
- "--advertise-client-urls=http://127.0.0.1:2379";,
- "--listen-client-urls=http://0.0.0.0:2379";,
- "--initial-advertise-peer-urls=http://127.0.0.1:2380";,
- "--listen-peer-urls=http://0.0.0.0:2380";,
- "--initial-cluster=etcd0=http://127.0.0.1:2380";,
- ]
+ ETCDCTL_API: "3"
+ command:
+ - etcd
+ - --name=etcd0
+ - --data-dir=/etcd-data
+ - --listen-client-urls=http://0.0.0.0:2379
+ - --advertise-client-urls=http://etcd:2379
+ - --listen-peer-urls=http://0.0.0.0:2380
+ - --initial-advertise-peer-urls=http://etcd:2380
+ - --initial-cluster=etcd0=http://etcd:2380
Review Comment:
The initial-advertise-peer-urls and initial-cluster have been changed to use
the Docker hostname `etcd` instead of `127.0.0.1`. While this change is
generally acceptable for internal peer communication within the Docker network,
it should be consistent with the advertise-client-urls change. Since the
advertise-client-urls needs to be reverted to `127.0.0.1` or `0.0.0.0` for
external client access, consider reverting these peer URLs as well for
consistency, or ensure the etcd container can resolve the `etcd` hostname
properly (which it should via Docker networking).
```suggestion
- --advertise-client-urls=http://127.0.0.1:2379
- --listen-peer-urls=http://0.0.0.0:2380
- --initial-advertise-peer-urls=http://127.0.0.1:2380
- --initial-cluster=etcd0=http://127.0.0.1:2380
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
Review Comment:
The health check retry count has been reduced from 24 retries (120 seconds)
to 3 retries (15 seconds) plus start_period. For Nacos, this gives a total of
45 seconds (30s start_period + 15s retries) compared to the original 120
seconds. If Nacos takes longer to start in certain environments (CI,
resource-constrained systems), this may cause integration tests to fail
prematurely. Consider increasing retries to at least 10 to allow more time for
service initialization, especially in CI environments where resources may be
limited.
```suggestion
retries: 10
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 30s
+ restart: unless-stopped
+ networks: [itnet]
polaris:
- image: polarismesh/polaris-standalone:latest
- container_name: polaris-standalone
+ image: polarismesh/polaris-standalone:v1.17.2
privileged: true
ports:
- "8090:8090"
- "8091:8091"
- "8093:8093"
healthcheck:
- test: "curl --fail http://127.0.0.1:8090 || exit 1"
+ test: [ "CMD", "curl", "-fsS", "http://localhost:8090/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 20s
+ restart: unless-stopped
+ networks: [itnet]
etcd:
- image: "quay.io/coreos/etcd:v3.4.28"
- container_name: etcd
+ image: quay.io/coreos/etcd:v3.4.28
environment:
- - ETCDCTL_API=3
- command: [
- "etcd",
- "--name=etcd0",
- "--advertise-client-urls=http://127.0.0.1:2379";,
- "--listen-client-urls=http://0.0.0.0:2379";,
- "--initial-advertise-peer-urls=http://127.0.0.1:2380";,
- "--listen-peer-urls=http://0.0.0.0:2380";,
- "--initial-cluster=etcd0=http://127.0.0.1:2380";,
- ]
+ ETCDCTL_API: "3"
+ command:
+ - etcd
+ - --name=etcd0
+ - --data-dir=/etcd-data
+ - --listen-client-urls=http://0.0.0.0:2379
+ - --advertise-client-urls=http://etcd:2379
+ - --listen-peer-urls=http://0.0.0.0:2380
+ - --initial-advertise-peer-urls=http://etcd:2380
+ - --initial-cluster=etcd0=http://etcd:2380
+ - --initial-cluster-state=new
ports:
- "2379:2379"
- "2380:2380"
- restart: always
+ volumes:
+ - etcd-data:/etcd-data
+ healthcheck:
+ test: [ "CMD", "etcdctl", "--endpoints=http://localhost:2379";,
"endpoint", "health" ]
+ interval: 5s
+ timeout: 3s
+ retries: 3
Review Comment:
The health check retry count has been reduced from 24 retries (120 seconds)
to 3 retries (15 seconds) plus start_period. For etcd, this gives a total of 25
seconds (10s start_period + 15s retries) compared to the original 120 seconds.
If etcd takes longer to start in certain environments (CI, resource-constrained
systems), this may cause integration tests to fail prematurely. Consider
increasing retries to at least 10 to allow more time for service initialization.
```suggestion
retries: 10
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
+ start_period: 30s
+ restart: unless-stopped
+ networks: [itnet]
polaris:
- image: polarismesh/polaris-standalone:latest
- container_name: polaris-standalone
+ image: polarismesh/polaris-standalone:v1.17.2
privileged: true
ports:
- "8090:8090"
- "8091:8091"
- "8093:8093"
healthcheck:
- test: "curl --fail http://127.0.0.1:8090 || exit 1"
+ test: [ "CMD", "curl", "-fsS", "http://localhost:8090/"; ]
interval: 5s
+ timeout: 3s
+ retries: 3
Review Comment:
The health check retry count has been reduced from 24 retries (120 seconds)
to 3 retries (15 seconds) plus start_period. For Polaris, this gives a total of
35 seconds (20s start_period + 15s retries) compared to the original 120
seconds. If Polaris takes longer to start in certain environments (CI,
resource-constrained systems), this may cause integration tests to fail
prematurely. Consider increasing retries to at least 10 to allow more time for
service initialization.
```suggestion
retries: 10
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
Review Comment:
There appears to be trailing whitespace at the end of these lines. While
this doesn't affect functionality, it's good practice to remove trailing
whitespace for cleaner code.
```suggestion
- "8848:8848"
- "9848:9848"
```
##########
docker-compose.yml:
##########
@@ -1,52 +1,83 @@
# services config in one docker-compose file for integrate test
-# integrate test will start up services and samples test will depend on those
containers
services:
zookeeper:
- image: zookeeper:3.8.4
+ image: zookeeper:3.9.4
ports:
- "2181:2181"
- restart: on-failure
+ healthcheck:
+ test: ["CMD", "nc", "-z", "localhost", "2181"]
+ interval: 5s
+ timeout: 3s
+ retries: 10
+ start_period: 10s
+ restart: unless-stopped
+ networks: [itnet]
nacos:
- image: nacos/nacos-server:v2.1.2-slim
- container_name: nacos-standalone
+ image: nacos/nacos-server:v2.5.2
environment:
- - PREFER_HOST_MODE=hostname
- - MODE=standalone
+ PREFER_HOST_MODE: hostname
+ MODE: standalone
ports:
- - "8848:8848"
- - "9848:9848"
+ - "8848:8848"
+ - "9848:9848"
+ - "9849:9849"
healthcheck:
- test: "curl --fail
http://127.0.0.1:8848/nacos/v1/console/health/liveness || exit 1"
+ test: [ "CMD", "curl", "-f", "http://localhost:8848/nacos/"; ]
Review Comment:
The health check endpoint has changed from
`/nacos/v1/console/health/liveness` to `/nacos/`. While `/nacos/` may work, the
previous endpoint was specifically designed for health checks (liveness probe).
Consider using the dedicated health check endpoint to ensure proper service
readiness validation. The Nacos v2.x API typically provides
`/nacos/v1/console/health/liveness` and `/nacos/v1/console/health/readiness`
endpoints for this purpose.
```suggestion
test: [ "CMD", "curl", "-f",
"http://localhost:8848/nacos/v1/console/health/liveness"; ]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
