Copilot commented on code in PR #62202:
URL: https://github.com/apache/doris/pull/62202#discussion_r3061624855
##########
regression-test/pipeline/common/github-utils.sh:
##########
@@ -427,12 +438,97 @@ github_utils__maybe_enable_external_stage_timer() {
([[ "${main_definition}" != *"collect_docker_logs"* ]] &&
[[ "${main_definition}" != *"COLLECT DOCKER LOGS"* ]]) ||
[[ "${main_definition}" != *"deploy_cluster.sh"* ]]; then
+ return 1
+ fi
+ return 0
+}
+
+github_utils__maybe_optimize_external_collect_docker_logs() {
+ local original_definition
+
+ if ! github_utils__is_external_inline_shell; then
+ return 0
+ fi
+ if ! declare -F collect_docker_logs >/dev/null; then
+ return 0
+ fi
+ if [[ "${GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED:-false}" ==
"true" ]]; then
return 0
fi
- # shellcheck source=/dev/null
- source "${timer_script}"
- external_regression_stage_timer_enable_auto_hooks
+ original_definition="$(declare -f collect_docker_logs)"
+ eval
"${original_definition/collect_docker_logs/github_utils__original_collect_docker_logs}"
+ collect_docker_logs() {
+ local collect_on_success="${COLLECT_DOCKER_LOGS_ON_SUCCESS:-false}"
+ local skip_reason=""
+
+ if [[ "${collect_on_success,,}" != "true" ]]; then
+ if
skip_reason="$(github_utils__external_collect_docker_logs_skip_reason)"; then
+ echo "Skip collecting docker logs on ${skip_reason}. Set
COLLECT_DOCKER_LOGS_ON_SUCCESS=true to enable."
+ return 0
+ fi
+ fi
+
+ github_utils__original_collect_docker_logs "$@"
+ }
+ GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED=true
+}
+
+github_utils__external_collect_docker_logs_skip_reason() {
+ local summary=""
+ local test_suites=0
+ local failed_suites=0
+ local fatal_scripts=0
+ local threshold="${failed_suites_threshold:-20}"
+
+ if [[ "${exit_flag:-0}" == "0" ]]; then
+ printf 'success'
+ return 0
+ fi
+
+ summary="$(github_utils__external_regression_summary_line)" || return 1
+ test_suites="$(echo "${summary}" | cut -d ' ' -f 2)"
+ failed_suites="$(echo "${summary}" | cut -d ' ' -f 5)"
+ fatal_scripts="$(echo "${summary}" | cut -d ' ' -f 8)"
+ if [[ ${test_suites} -gt 0 && ${failed_suites} -le ${threshold} &&
${fatal_scripts} -eq 0 ]]; then
+ printf 'tolerated success'
+ return 0
+ fi
+ return 1
+}
+
+github_utils__external_regression_summary_line() {
+ local summary_log
+
+ summary_log="$(github_utils__external_regression_summary_log)" || return 1
+ grep -aoE 'Test ([0-9]+) suites, failed ([0-9]+) suites, fatal ([0-9]+)
scripts, skipped ([0-9]+) scripts' \
+ "${summary_log}" | tail -n 1
+}
+
+github_utils__external_regression_summary_log() {
+ local log_dir=""
+ local candidate=""
+
+ if [[ -n "${DORIS_HOME:-}" ]]; then
+ log_dir="${DORIS_HOME}/regression-test/log"
+ candidate="$(find "${log_dir}" -maxdepth 1 -type f -name
'doris-regression-test.*.log' | head -n 1)"
+ if [[ -n "${candidate}" ]]; then
+ printf '%s' "${candidate}"
+ return 0
+ fi
+ fi
+
+ if [[ -n "${case_center:-}" && -n "${cluster_name:-}" ]]; then
+ log_dir="${case_center}/${cluster_name}/output/regression-test/log"
+ candidate="$(find "${log_dir}" -maxdepth 1 -type f -name
'doris-regression-test.*.log' | head -n 1)"
+ if [[ -n "${candidate}" ]]; then
+ printf '%s' "${candidate}"
+ return 0
+ fi
Review Comment:
Same issue as above for the case-center path: `find ... | head -n 1` is
non-deterministic when multiple regression logs exist, potentially parsing the
wrong summary and misclassifying tolerated success. Consider choosing the
newest log file and silencing `find` errors if the directory is missing.
##########
regression-test/pipeline/common/github-utils.sh:
##########
@@ -427,12 +438,97 @@ github_utils__maybe_enable_external_stage_timer() {
([[ "${main_definition}" != *"collect_docker_logs"* ]] &&
[[ "${main_definition}" != *"COLLECT DOCKER LOGS"* ]]) ||
[[ "${main_definition}" != *"deploy_cluster.sh"* ]]; then
+ return 1
+ fi
+ return 0
+}
+
+github_utils__maybe_optimize_external_collect_docker_logs() {
+ local original_definition
+
+ if ! github_utils__is_external_inline_shell; then
+ return 0
+ fi
+ if ! declare -F collect_docker_logs >/dev/null; then
+ return 0
+ fi
+ if [[ "${GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED:-false}" ==
"true" ]]; then
return 0
fi
- # shellcheck source=/dev/null
- source "${timer_script}"
- external_regression_stage_timer_enable_auto_hooks
+ original_definition="$(declare -f collect_docker_logs)"
+ eval
"${original_definition/collect_docker_logs/github_utils__original_collect_docker_logs}"
+ collect_docker_logs() {
+ local collect_on_success="${COLLECT_DOCKER_LOGS_ON_SUCCESS:-false}"
+ local skip_reason=""
+
+ if [[ "${collect_on_success,,}" != "true" ]]; then
+ if
skip_reason="$(github_utils__external_collect_docker_logs_skip_reason)"; then
+ echo "Skip collecting docker logs on ${skip_reason}. Set
COLLECT_DOCKER_LOGS_ON_SUCCESS=true to enable."
+ return 0
+ fi
+ fi
+
+ github_utils__original_collect_docker_logs "$@"
+ }
+ GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED=true
+}
+
+github_utils__external_collect_docker_logs_skip_reason() {
+ local summary=""
+ local test_suites=0
+ local failed_suites=0
+ local fatal_scripts=0
+ local threshold="${failed_suites_threshold:-20}"
+
+ if [[ "${exit_flag:-0}" == "0" ]]; then
+ printf 'success'
+ return 0
+ fi
+
+ summary="$(github_utils__external_regression_summary_line)" || return 1
+ test_suites="$(echo "${summary}" | cut -d ' ' -f 2)"
+ failed_suites="$(echo "${summary}" | cut -d ' ' -f 5)"
+ fatal_scripts="$(echo "${summary}" | cut -d ' ' -f 8)"
+ if [[ ${test_suites} -gt 0 && ${failed_suites} -le ${threshold} &&
${fatal_scripts} -eq 0 ]]; then
+ printf 'tolerated success'
+ return 0
+ fi
+ return 1
+}
+
+github_utils__external_regression_summary_line() {
+ local summary_log
+
+ summary_log="$(github_utils__external_regression_summary_log)" || return 1
+ grep -aoE 'Test ([0-9]+) suites, failed ([0-9]+) suites, fatal ([0-9]+)
scripts, skipped ([0-9]+) scripts' \
+ "${summary_log}" | tail -n 1
+}
+
+github_utils__external_regression_summary_log() {
+ local log_dir=""
+ local candidate=""
+
+ if [[ -n "${DORIS_HOME:-}" ]]; then
+ log_dir="${DORIS_HOME}/regression-test/log"
+ candidate="$(find "${log_dir}" -maxdepth 1 -type f -name
'doris-regression-test.*.log' | head -n 1)"
+ if [[ -n "${candidate}" ]]; then
+ printf '%s' "${candidate}"
+ return 0
+ fi
Review Comment:
`find ... | head -n 1` selects an arbitrary log file if multiple
`doris-regression-test.*.log` exist under `${DORIS_HOME}/regression-test/log`,
which can cause the tolerated-success logic to parse the wrong run and
skip/collect logs incorrectly. Prefer selecting the most recent matching log
(and consider suppressing `find` stderr when the directory is absent).
##########
regression-test/pipeline/common/github-utils.sh:
##########
@@ -427,12 +438,97 @@ github_utils__maybe_enable_external_stage_timer() {
([[ "${main_definition}" != *"collect_docker_logs"* ]] &&
[[ "${main_definition}" != *"COLLECT DOCKER LOGS"* ]]) ||
[[ "${main_definition}" != *"deploy_cluster.sh"* ]]; then
+ return 1
+ fi
+ return 0
+}
+
+github_utils__maybe_optimize_external_collect_docker_logs() {
+ local original_definition
+
+ if ! github_utils__is_external_inline_shell; then
+ return 0
+ fi
+ if ! declare -F collect_docker_logs >/dev/null; then
+ return 0
+ fi
+ if [[ "${GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED:-false}" ==
"true" ]]; then
return 0
fi
- # shellcheck source=/dev/null
- source "${timer_script}"
- external_regression_stage_timer_enable_auto_hooks
+ original_definition="$(declare -f collect_docker_logs)"
+ eval
"${original_definition/collect_docker_logs/github_utils__original_collect_docker_logs}"
+ collect_docker_logs() {
+ local collect_on_success="${COLLECT_DOCKER_LOGS_ON_SUCCESS:-false}"
+ local skip_reason=""
+
+ if [[ "${collect_on_success,,}" != "true" ]]; then
+ if
skip_reason="$(github_utils__external_collect_docker_logs_skip_reason)"; then
+ echo "Skip collecting docker logs on ${skip_reason}. Set
COLLECT_DOCKER_LOGS_ON_SUCCESS=true to enable."
+ return 0
+ fi
+ fi
+
+ github_utils__original_collect_docker_logs "$@"
+ }
+ GITHUB_UTILS_EXTERNAL_COLLECT_DOCKER_LOGS_WRAPPED=true
+}
+
+github_utils__external_collect_docker_logs_skip_reason() {
+ local summary=""
+ local test_suites=0
+ local failed_suites=0
+ local fatal_scripts=0
+ local threshold="${failed_suites_threshold:-20}"
+
+ if [[ "${exit_flag:-0}" == "0" ]]; then
+ printf 'success'
+ return 0
+ fi
+
+ summary="$(github_utils__external_regression_summary_line)" || return 1
+ test_suites="$(echo "${summary}" | cut -d ' ' -f 2)"
+ failed_suites="$(echo "${summary}" | cut -d ' ' -f 5)"
+ fatal_scripts="$(echo "${summary}" | cut -d ' ' -f 8)"
+ if [[ ${test_suites} -gt 0 && ${failed_suites} -le ${threshold} &&
${fatal_scripts} -eq 0 ]]; then
+ printf 'tolerated success'
+ return 0
+ fi
+ return 1
+}
+
+github_utils__external_regression_summary_line() {
+ local summary_log
+
+ summary_log="$(github_utils__external_regression_summary_log)" || return 1
+ grep -aoE 'Test ([0-9]+) suites, failed ([0-9]+) suites, fatal ([0-9]+)
scripts, skipped ([0-9]+) scripts' \
+ "${summary_log}" | tail -n 1
+}
Review Comment:
`github_utils__external_regression_summary_line` relies on the caller having
`set -o pipefail` enabled: if the grep finds no matches, `tail -n 1` still
exits 0 and this function can return an empty string as “success”. That can
later lead to invalid integer comparisons (or incorrect skip/collect
decisions). Make this function return non-zero when no summary line is found
(e.g., validate non-empty output or explicitly handle grep’s exit status).
--
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]
