Copilot commented on code in PR #1034:
URL: https://github.com/apache/dubbo-go-samples/pull/1034#discussion_r2806896236
##########
integrate_test.sh:
##########
@@ -14,73 +16,258 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-if [ -z "$1" ]; then
- echo "Provide test directory please, like : ./integrate_test.sh helloworld"
- exit
+set -euo pipefail
+
+if [ -z "${1:-}" ]; then
+ echo "Provide sample directory, like: ./integrate_test.sh direct"
+ exit 1
fi
-echo "=========================================="
-echo "Starting integration test for: $1"
-echo "Test project directory: $(pwd)/$1"
-echo "Integration test directory: $(pwd)/integrate_test/$1"
-echo "=========================================="
+SAMPLE="$1"
+P_DIR="$(pwd)/$SAMPLE"
+PROJECT_NAME="$(basename "$P_DIR")"
+GO_SERVER_LOG="/tmp/.${PROJECT_NAME}.go-server.log"
+JAVA_SERVER_LOG="/tmp/.${PROJECT_NAME}.java-server.log"
-P_DIR=$(pwd)/$1
-if [ -f "$P_DIR"/build/test.sh ]; then
- echo "Found custom test script, running: $P_DIR/build/test.sh"
- "$P_DIR"/build/test.sh "$P_DIR"
- result=$?
- exit $((result))
+if [ ! -d "$P_DIR" ]; then
+ echo "Sample directory not found: $P_DIR"
+ exit 1
fi
-INTEGRATE_DIR=$(pwd)/integrate_test/$1
+JAVA_SERVER_RUN_SH="$(find "$P_DIR" -type f -path '*/java-server*/run.sh'
-print -quit || true)"
+JAVA_CLIENT_RUN_SH="$(find "$P_DIR" -type f -path '*/java-client*/run.sh'
-print -quit || true)"
+JAVA_SERVER_PID=""
+GO_AUX_PIDS=()
+JAVA_ENABLED=true
+if { [ -n "$JAVA_SERVER_RUN_SH" ] || [ -n "$JAVA_CLIENT_RUN_SH" ]; } && !
command -v mvn >/dev/null 2>&1; then
+ JAVA_ENABLED=false
+ echo "Maven (mvn) is not available, all Java phases will be skipped for
sample: $SAMPLE"
+fi
-echo "Using project directory: $P_DIR"
-echo "Using integration directory: $INTEGRATE_DIR"
+cleanup() {
+ local aux_pid
+ for aux_pid in "${GO_AUX_PIDS[@]:-}"; do
+ if [ -n "$aux_pid" ] && kill -0 "$aux_pid" 2>/dev/null; then
+ kill "$aux_pid" 2>/dev/null || true
+ sleep 1
+ kill -9 "$aux_pid" 2>/dev/null || true
+ fi
+ done
Review Comment:
The cleanup function iterates over `GO_AUX_PIDS` array which could be empty
or unset. While the loop handles this with `"${GO_AUX_PIDS[@]:-}"`, the empty
expansion could still cause issues in strict mode. The current implementation
is correct, but consider adding an explicit check `if [ "${#GO_AUX_PIDS[@]}"
-gt 0 ]; then` before the loop for clarity and to avoid potential edge cases in
different bash versions.
```suggestion
if [ "${#GO_AUX_PIDS[@]}" -gt 0 ]; then
for aux_pid in "${GO_AUX_PIDS[@]}"; do
if [ -n "$aux_pid" ] && kill -0 "$aux_pid" 2>/dev/null; then
kill "$aux_pid" 2>/dev/null || true
sleep 1
kill -9 "$aux_pid" 2>/dev/null || true
fi
done
fi
```
##########
integrate_test.sh:
##########
@@ -14,73 +16,258 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-if [ -z "$1" ]; then
- echo "Provide test directory please, like : ./integrate_test.sh helloworld"
- exit
+set -euo pipefail
+
+if [ -z "${1:-}" ]; then
+ echo "Provide sample directory, like: ./integrate_test.sh direct"
+ exit 1
fi
-echo "=========================================="
-echo "Starting integration test for: $1"
-echo "Test project directory: $(pwd)/$1"
-echo "Integration test directory: $(pwd)/integrate_test/$1"
-echo "=========================================="
+SAMPLE="$1"
+P_DIR="$(pwd)/$SAMPLE"
+PROJECT_NAME="$(basename "$P_DIR")"
+GO_SERVER_LOG="/tmp/.${PROJECT_NAME}.go-server.log"
+JAVA_SERVER_LOG="/tmp/.${PROJECT_NAME}.java-server.log"
-P_DIR=$(pwd)/$1
-if [ -f "$P_DIR"/build/test.sh ]; then
- echo "Found custom test script, running: $P_DIR/build/test.sh"
- "$P_DIR"/build/test.sh "$P_DIR"
- result=$?
- exit $((result))
+if [ ! -d "$P_DIR" ]; then
+ echo "Sample directory not found: $P_DIR"
+ exit 1
fi
-INTEGRATE_DIR=$(pwd)/integrate_test/$1
+JAVA_SERVER_RUN_SH="$(find "$P_DIR" -type f -path '*/java-server*/run.sh'
-print -quit || true)"
+JAVA_CLIENT_RUN_SH="$(find "$P_DIR" -type f -path '*/java-client*/run.sh'
-print -quit || true)"
+JAVA_SERVER_PID=""
+GO_AUX_PIDS=()
+JAVA_ENABLED=true
+if { [ -n "$JAVA_SERVER_RUN_SH" ] || [ -n "$JAVA_CLIENT_RUN_SH" ]; } && !
command -v mvn >/dev/null 2>&1; then
+ JAVA_ENABLED=false
+ echo "Maven (mvn) is not available, all Java phases will be skipped for
sample: $SAMPLE"
+fi
-echo "Using project directory: $P_DIR"
-echo "Using integration directory: $INTEGRATE_DIR"
+cleanup() {
+ local aux_pid
+ for aux_pid in "${GO_AUX_PIDS[@]:-}"; do
+ if [ -n "$aux_pid" ] && kill -0 "$aux_pid" 2>/dev/null; then
+ kill "$aux_pid" 2>/dev/null || true
+ sleep 1
+ kill -9 "$aux_pid" 2>/dev/null || true
+ fi
+ done
-# waiting for port release
-sleep 5
+ if [ -n "$JAVA_SERVER_PID" ] && kill -0 "$JAVA_SERVER_PID" 2>/dev/null; then
+ kill "$JAVA_SERVER_PID" 2>/dev/null || true
+ sleep 1
+ kill -9 "$JAVA_SERVER_PID" 2>/dev/null || true
+ fi
-# start server
-echo "Starting server..."
-make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile start
-# waiting for registry
-sleep 5
+ make PROJECT_DIR="$P_DIR" PROJECT_NAME="$PROJECT_NAME" -f Makefile stop
>/dev/null 2>&1 || true
+}
+trap cleanup EXIT
-# start integration
-echo "Running Go integration tests..."
-make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile integration
-result=$?
+resolve_config_path() {
+ local role="$1"
+ local conf_dir="$P_DIR/$role/conf"
+ if [ -f "$conf_dir/dubbogo.yml" ]; then
+ echo "$conf_dir/dubbogo.yml"
+ return 0
+ fi
+ if [ -f "$conf_dir/dubbogo.yaml" ]; then
+ echo "$conf_dir/dubbogo.yaml"
+ return 0
+ fi
+ return 1
+}
-# if fail print server log
-if [ $result != 0 ];then
- echo "Go integration test failed, printing server log..."
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile print-server-log
-fi
+run_go_client() {
+ if ! compgen -G "$P_DIR/go-client/cmd/*.go" >/dev/null; then
+ echo "go-client/cmd/*.go not found in $P_DIR"
+ return 1
+ fi
+
+ local timeout_seconds="${GO_CLIENT_TIMEOUT_SECONDS:-90}"
+
+ local client_conf
+ client_conf="$(resolve_config_path "go-client" || true)"
+
+ echo "Running Go client..."
+ (
+ cd "$P_DIR"
+ if [ -n "$client_conf" ]; then
+ export DUBBO_GO_CONFIG_PATH="$client_conf"
+ fi
+ go run ./go-client/cmd/*.go
+ ) &
+ local go_client_pid=$!
+ local elapsed=0
-JAVA_TEST_SHELL=$INTEGRATE_DIR/tests/java
-if [ -e $JAVA_TEST_SHELL ]; then
- echo "Found Java tests, running Java integration tests..."
- # run java test
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile integration-java
- result=$?
-
- # if fail print server log
- if [ $result != 0 ];then
- echo "Java integration test failed, printing server log..."
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile print-server-log
+ while kill -0 "$go_client_pid" 2>/dev/null; do
+ if [ "$elapsed" -ge "$timeout_seconds" ]; then
+ echo "Go client timed out after ${timeout_seconds}s: $SAMPLE"
+ kill "$go_client_pid" 2>/dev/null || true
+ sleep 1
+ kill -9 "$go_client_pid" 2>/dev/null || true
+ wait "$go_client_pid" 2>/dev/null || true
+ return 124
+ fi
+ sleep 1
+ elapsed=$((elapsed + 1))
+ done
+
+ wait "$go_client_pid"
+}
+
+start_aux_go_servers() {
+ local aux_server_dir
+ local aux_name
+ local aux_log
+ local aux_pid
+ local elapsed
+
+ while IFS= read -r aux_server_dir; do
+ [ -z "$aux_server_dir" ] && continue
+ aux_name="$(basename "$(dirname "$aux_server_dir")")"
+ aux_log="/tmp/.${PROJECT_NAME}.${aux_name}.log"
+
+ echo "Starting auxiliary Go server: $aux_name"
+ (
+ cd "$P_DIR"
+ go run "./${aux_server_dir#"$P_DIR"/}"/*.go
+ ) >"$aux_log" 2>&1 &
+ aux_pid="$!"
+ GO_AUX_PIDS+=("$aux_pid")
+
+ elapsed=0
+ while kill -0 "$aux_pid" 2>/dev/null; do
+ if [ "$elapsed" -ge 10 ]; then
+ break
+ fi
+ sleep 1
+ elapsed=$((elapsed + 1))
+ done
+
+ if ! kill -0 "$aux_pid" 2>/dev/null; then
+ echo "Auxiliary Go server exited unexpectedly: $aux_name"
+ cat "$aux_log" || true
+ return 1
+ fi
+ done < <(find "$P_DIR" -mindepth 1 -maxdepth 1 -type d -name '*-server' !
-name 'go-server' ! -name 'java-server' -exec sh -c 'test -d "$1/cmd" && ls
"$1"/cmd/*.go >/dev/null 2>&1 && echo "$1/cmd"' _ {} \;)
Review Comment:
The find command on line 153 uses a complex shell invocation that may not be
portable. The pattern `! -name 'go-server' ! -name 'java-server'` correctly
excludes these directories, but the nested `sh -c` with multiple tests could
fail silently in some edge cases. Consider adding error handling or simplifying
the logic. Additionally, the find command could match directories like
`test-server` or `mock-server` unintentionally. Consider using a more specific
pattern like `-name '*-server' -a \( -name 'grpc-server' -o -name
'auxiliary-server' \)` if only specific auxiliary servers are expected.
##########
integrate_test.sh:
##########
@@ -14,73 +16,258 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-if [ -z "$1" ]; then
- echo "Provide test directory please, like : ./integrate_test.sh helloworld"
- exit
+set -euo pipefail
+
+if [ -z "${1:-}" ]; then
+ echo "Provide sample directory, like: ./integrate_test.sh direct"
+ exit 1
fi
-echo "=========================================="
-echo "Starting integration test for: $1"
-echo "Test project directory: $(pwd)/$1"
-echo "Integration test directory: $(pwd)/integrate_test/$1"
-echo "=========================================="
+SAMPLE="$1"
+P_DIR="$(pwd)/$SAMPLE"
+PROJECT_NAME="$(basename "$P_DIR")"
+GO_SERVER_LOG="/tmp/.${PROJECT_NAME}.go-server.log"
+JAVA_SERVER_LOG="/tmp/.${PROJECT_NAME}.java-server.log"
-P_DIR=$(pwd)/$1
-if [ -f "$P_DIR"/build/test.sh ]; then
- echo "Found custom test script, running: $P_DIR/build/test.sh"
- "$P_DIR"/build/test.sh "$P_DIR"
- result=$?
- exit $((result))
+if [ ! -d "$P_DIR" ]; then
+ echo "Sample directory not found: $P_DIR"
+ exit 1
fi
-INTEGRATE_DIR=$(pwd)/integrate_test/$1
+JAVA_SERVER_RUN_SH="$(find "$P_DIR" -type f -path '*/java-server*/run.sh'
-print -quit || true)"
+JAVA_CLIENT_RUN_SH="$(find "$P_DIR" -type f -path '*/java-client*/run.sh'
-print -quit || true)"
+JAVA_SERVER_PID=""
+GO_AUX_PIDS=()
+JAVA_ENABLED=true
+if { [ -n "$JAVA_SERVER_RUN_SH" ] || [ -n "$JAVA_CLIENT_RUN_SH" ]; } && !
command -v mvn >/dev/null 2>&1; then
+ JAVA_ENABLED=false
+ echo "Maven (mvn) is not available, all Java phases will be skipped for
sample: $SAMPLE"
+fi
-echo "Using project directory: $P_DIR"
-echo "Using integration directory: $INTEGRATE_DIR"
+cleanup() {
+ local aux_pid
+ for aux_pid in "${GO_AUX_PIDS[@]:-}"; do
+ if [ -n "$aux_pid" ] && kill -0 "$aux_pid" 2>/dev/null; then
+ kill "$aux_pid" 2>/dev/null || true
+ sleep 1
+ kill -9 "$aux_pid" 2>/dev/null || true
+ fi
+ done
-# waiting for port release
-sleep 5
+ if [ -n "$JAVA_SERVER_PID" ] && kill -0 "$JAVA_SERVER_PID" 2>/dev/null; then
+ kill "$JAVA_SERVER_PID" 2>/dev/null || true
+ sleep 1
+ kill -9 "$JAVA_SERVER_PID" 2>/dev/null || true
+ fi
-# start server
-echo "Starting server..."
-make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile start
-# waiting for registry
-sleep 5
+ make PROJECT_DIR="$P_DIR" PROJECT_NAME="$PROJECT_NAME" -f Makefile stop
>/dev/null 2>&1 || true
+}
+trap cleanup EXIT
-# start integration
-echo "Running Go integration tests..."
-make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile integration
-result=$?
+resolve_config_path() {
+ local role="$1"
+ local conf_dir="$P_DIR/$role/conf"
+ if [ -f "$conf_dir/dubbogo.yml" ]; then
+ echo "$conf_dir/dubbogo.yml"
+ return 0
+ fi
+ if [ -f "$conf_dir/dubbogo.yaml" ]; then
+ echo "$conf_dir/dubbogo.yaml"
+ return 0
+ fi
+ return 1
+}
-# if fail print server log
-if [ $result != 0 ];then
- echo "Go integration test failed, printing server log..."
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile print-server-log
-fi
+run_go_client() {
+ if ! compgen -G "$P_DIR/go-client/cmd/*.go" >/dev/null; then
+ echo "go-client/cmd/*.go not found in $P_DIR"
+ return 1
+ fi
+
+ local timeout_seconds="${GO_CLIENT_TIMEOUT_SECONDS:-90}"
+
+ local client_conf
+ client_conf="$(resolve_config_path "go-client" || true)"
+
+ echo "Running Go client..."
+ (
+ cd "$P_DIR"
+ if [ -n "$client_conf" ]; then
+ export DUBBO_GO_CONFIG_PATH="$client_conf"
+ fi
+ go run ./go-client/cmd/*.go
+ ) &
+ local go_client_pid=$!
+ local elapsed=0
-JAVA_TEST_SHELL=$INTEGRATE_DIR/tests/java
-if [ -e $JAVA_TEST_SHELL ]; then
- echo "Found Java tests, running Java integration tests..."
- # run java test
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile integration-java
- result=$?
-
- # if fail print server log
- if [ $result != 0 ];then
- echo "Java integration test failed, printing server log..."
- make PROJECT_DIR=$P_DIR PROJECT_NAME=$(basename $P_DIR)
INTEGRATE_DIR=$INTEGRATE_DIR -f build/Makefile print-server-log
+ while kill -0 "$go_client_pid" 2>/dev/null; do
+ if [ "$elapsed" -ge "$timeout_seconds" ]; then
+ echo "Go client timed out after ${timeout_seconds}s: $SAMPLE"
+ kill "$go_client_pid" 2>/dev/null || true
+ sleep 1
+ kill -9 "$go_client_pid" 2>/dev/null || true
+ wait "$go_client_pid" 2>/dev/null || true
+ return 124
+ fi
+ sleep 1
+ elapsed=$((elapsed + 1))
+ done
Review Comment:
The timeout logic for go-client uses a polling loop that checks every
second. This means the actual timeout could be up to 1 second longer than
specified (if the process exits just before the timeout check). Consider using
`timeout` command instead for more precise control: `timeout "$timeout_seconds"
go run ./go-client/cmd/*.go` would be simpler and more accurate.
##########
.github/workflows/github-actions.yml:
##########
@@ -2,90 +2,89 @@ name: CI
on:
push:
- branches:
- - main
- - 'release-*'
- - 'feature-*'
pull_request:
- branches: "*"
-jobs:
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
+ cancel-in-progress: true
- build:
- name: ${{ matrix.os }} - Go ${{ matrix.go_version }}
- runs-on: ${{ matrix.os }}
- strategy:
- # If you want to matrix build, you can append the following list.
- matrix:
- go_version:
- - '1.24'
- os:
- - ubuntu-latest
+jobs:
+ build-and-integration:
+ name: Build And Integration
+ runs-on: ubuntu-latest
env:
DING_TOKEN: ${{ secrets.DING_TOKEN }}
DING_SIGN: ${{ secrets.DING_SIGN }}
steps:
- - name: Set up Go 1.x
- uses: actions/setup-go@v2
- with:
- go-version: ${{ matrix.go_version }}
- id: go
+ - name: Check out code
+ uses: actions/checkout@v5
- - name: Check out code into the Go module directory
- uses: actions/[email protected]
+ - name: Set up Go
+ uses: actions/setup-go@v6
+ with:
+ go-version-file: go.mod
+ cache: true
- - name: Cache dependencies
+ - name: Cache Go build artifacts
uses: actions/cache@v4
with:
- # Cache
- path: ~/go/pkg/mod
- # Cache key
- key: ${{ runner.os }}-go-${{ matrix.go_version }}-${{
hashFiles('**/go.sum') }}
- # An ordered list of keys to use for restoring the cache if no cache
hit occurred for key
+ path: |
+ ~/.cache/go-build
+ ~/go/bin
+ key: ${{ runner.os }}-go-build-bin-${{ hashFiles('**/go.sum') }}
restore-keys: |
- ${{ runner.os }}-go-${{ matrix.go_version }}-
+ ${{ runner.os }}-go-build-bin-
- - name: Get dependencies
- run: |
- go mod tidy
- # for imports-formatter
- go install github.com/dubbogo/tools/cmd/imports-formatter@latest
+ - name: Set up Java
+ uses: actions/setup-java@v4
+ with:
+ distribution: temurin
+ java-version: '17'
+ cache: maven
- - name: Format Code
+ - name: Check tool versions
run: |
- go fmt ./... && GOROOT=$(go env GOROOT) imports-formatter && git
status && [[ -z `git status -s` ]]
- # diff -u <(echo -n) <(gofmt -d -s .)
+ go version
+ java -version
+ mvn -version
- - name: Check License Header
- uses:
apache/skywalking-eyes/header@e1a02359b239bd28de3f6d35fdc870250fa513d5
+ - name: Install imports-formatter
+ run: |
+ command -v imports-formatter >/dev/null 2>&1 || go install
github.com/dubbogo/tools/cmd/imports-formatter@latest
- - name: Set Go version again to override skywalking
- uses: actions/setup-go@v2
- with:
- go-version: ${{ matrix.go_version }}
+ - name: Get dependencies
+ run: |
+ go mod tidy
- - name: Check Go version
+ - name: Format code
run: |
- go version
+ go fmt ./...
+ GOROOT=$(go env GOROOT) imports-formatter
+ git status
+ [[ -z "$(git status -s)" ]]
+
+ - name: Check license header
+ uses: apache/skywalking-eyes/header@main
Review Comment:
The license header check now uses `@main` instead of a pinned commit hash.
This introduces a risk of the action behavior changing unexpectedly if the main
branch of apache/skywalking-eyes is updated. Consider pinning to a specific
version or commit hash for reproducibility and stability, as was done
previously with `@e1a02359b239bd28de3f6d35fdc870250fa513d5`.
```suggestion
uses:
apache/skywalking-eyes/header@e1a02359b239bd28de3f6d35fdc870250fa513d5
```
##########
docker-health-check.sh:
##########
@@ -0,0 +1,53 @@
+#!/bin/bash
+
+set -euo pipefail
+
+retry() {
+ local name="$1"
+ local attempts="$2"
+ local interval="$3"
+ shift 3
+
+ local i
+ for ((i = 1; i <= attempts; i++)); do
+ if "$@"; then
+ echo "[health-check] $name is ready"
+ return 0
+ fi
+ echo "[health-check] waiting for $name ($i/$attempts)..."
+ sleep "$interval"
+ done
+
+ echo "[health-check] $name is not ready after $attempts attempts"
+ return 1
+}
+
+check_zookeeper() {
+ # zookeeper 2181 is not HTTP; "empty reply" also indicates port is open.
+ curl -sS --max-time 2 127.0.0.1:2181 >/dev/null 2>&1
+ local code=$?
+ [ "$code" -eq 0 ] || [ "$code" -eq 52 ]
Review Comment:
The comment at line 26 mentions "zookeeper 2181 is not HTTP" and explains
that an empty reply also indicates the port is open. However, the logic checks
for exit code 52 OR 0. This is correct (curl exit code 52 is "empty reply from
server"), but the comment could be clearer. Consider: "zookeeper 2181 is not
HTTP; curl returns 52 (empty reply) when the port is open and responding."
##########
start_integrate_test.sh:
##########
@@ -1,3 +1,5 @@
+#!/bin/bash
Review Comment:
The PR title contains a spelling error: "intergrate" should be "integrate".
--
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]
