kaiwangleo commented on code in PR #996:
URL: https://github.com/apache/flink-agents/pull/996#discussion_r3764094081
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
+ elif command -v wget >/dev/null 2>&1; then
+ if ! wget --quiet --output-document="$JAR_DL" "$URL"; then
Review Comment:
Agreed. I changed wget from --quiet to --no-verbose, which keeps normal
progress output suppressed while preserving error and basic diagnostic
messages. The download-failure path remains covered by the focused tests in
commit 585770e6.
##########
tools/test/unit/check_license.bats:
##########
@@ -0,0 +1,143 @@
+#!/usr/bin/env bats
+
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+command() {
+ if [[ "$1" == "-v" ]]; then
+ local missing
+ for missing in "${MISSING_COMMANDS[@]:-}"; do
+ if [[ "$2" == "$missing" ]]; then
+ return 1
+ fi
+ done
+ fi
+ builtin command "$@"
+}
+
+shim_bin() {
+ local name="$1" exit_code="${2:-0}"
+ cat >"$SHIM_DIR/$name" <<EOF
+#!/usr/bin/env bash
+( IFS=\$'\t'; printf '%s\n' "\$*" ) >> "$SHIM_CALLS/$name.log"
+exit $exit_code
+EOF
+ chmod +x "$SHIM_DIR/$name"
+}
+
+shim_bin_script() {
+ local name="$1" body="$2"
+ cat >"$SHIM_DIR/$name" <<EOF
+#!/usr/bin/env bash
+( IFS=\$'\t'; printf '%s\n' "\$*" ) >> "$SHIM_CALLS/$name.log"
+$body
+EOF
+ chmod +x "$SHIM_DIR/$name"
+}
+
+shim_bin_missing() {
+ MISSING_COMMANDS+=("$1")
+}
+
+shim_call_count() {
+ local log="$SHIM_CALLS/$1.log"
+ if [[ ! -f "$log" ]]; then
+ echo 0
+ return
+ fi
+ wc -l <"$log" | tr -d ' '
+}
+
+setup() {
+ CHECK_LICENSE_SOURCE_ONLY=1
+ source "${BATS_TEST_DIRNAME}/../../check-license.sh"
+ unset CHECK_LICENSE_SOURCE_ONLY
+
+ SHIM_DIR="$BATS_TEST_TMPDIR/bin"
+ SHIM_CALLS="$BATS_TEST_TMPDIR/calls"
+ MISSING_COMMANDS=()
+ mkdir -p "$SHIM_DIR" "$SHIM_CALLS"
+ export PATH="$SHIM_DIR:$PATH"
+
+ RAT_VERSION="test"
+ rat_jar="$BATS_TEST_TMPDIR/apache-rat-test.jar"
+ JAR="$rat_jar"
+ JAVA_HOME="$BATS_TEST_TMPDIR/missing-java-home"
+}
+
+@test "reports missing validation tools without deleting an existing JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin_missing unzip
+
+ run acquire_rat_jar
+
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"install a JDK with 'jar' or install 'unzip'"* ]]
+ [ -f "$rat_jar" ]
+}
+
+@test "rejects and removes an invalid existing JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin unzip 1
+
+ run acquire_rat_jar
+
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"is invalid"* ]]
+ [ ! -f "$rat_jar" ]
+}
+
+@test "does not download an existing valid JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin unzip
+ shim_bin curl
+
+ acquire_rat_jar
+
+ [ "$(shim_call_count curl)" = "0" ]
+ [ "$(shim_call_count unzip)" = "1" ]
+}
+
+@test "reports a download failure and removes the partial file" {
+ shim_bin curl 22
Review Comment:
Thanks for identifying the vacuous assertion. The curl failure stub now
creates the requested .part output file before exiting 22, so the [ ! -e
\.part\ ] assertion verifies the cleanup performed by the script rather than
passing trivially. The focused test suite passes in commit 585770e6.
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
Review Comment:
Good catch. --show-error is now paired with --silent, so curl suppresses the
progress meter while still surfacing useful error text together with --fail.
The focused download tests pass in commit 585770e6.
##########
tools/test/unit/check_license.bats:
##########
@@ -0,0 +1,143 @@
+#!/usr/bin/env bats
+
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+command() {
Review Comment:
Yes, reusing the shared helper is cleaner. I removed the duplicated shim
implementations and now load ../helpers/shim and call shim_setup from the test
setup. The five focused tests remain green in commit 585770e6.
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
+ elif command -v wget >/dev/null 2>&1; then
+ if ! wget --quiet --output-document="$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
else
- printf "You do not have curl or wget installed, please install rat
manually.\n"
- exit -1
+ printf "Cannot download Apache RAT: install 'curl' or 'wget'.\n" >&2
+ return 1
+ fi
+ if ! mv "$JAR_DL" "$JAR"; then
+ rm -f "$JAR_DL"
+ printf "Failed to store the downloaded Apache RAT JAR at %s.\n" "$JAR"
>&2
+ return 1
fi
fi
- unzip -tq "$JAR" &> /dev/null
- if [ $? -ne 0 ]; then
- # We failed to download
- rm "$JAR"
- printf "Our attempt to download rat locally to ${JAR} failed. Please
install rat manually.\n"
- exit -1
+ validate_rat_jar
+ validation_status=$?
+ if [ "$validation_status" -eq 2 ]; then
Review Comment:
Thanks for reproducing this edge case. I agree that a cached RAT JAR should
remain usable when validation tools are unavailable, especially because the CI
license step runs before the workflow installs a JDK. The script now
distinguishes cached from newly downloaded JARs: an unavailable validator emits
a warning and proceeds for a cached JAR, while a newly downloaded JAR fails
closed and is removed because it has not been validated. Invalid JARs still
fail in both cases. This is covered by the updated cached-JAR test in commit
585770e6.
--
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]
