weiqingy commented on code in PR #996:
URL: https://github.com/apache/flink-agents/pull/996#discussion_r3780473082
##########
tools/check-license.sh:
##########
@@ -20,41 +20,83 @@
# 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
Review Comment:
All four tests that reach validation shim `jar` as missing
(`check_license.bats:37`, `49`, `61`, `84`) and point `JAVA_HOME` at a
nonexistent directory, so neither `jar` arm ever runs and `unzip` ends up the
only validator the suite covers. That is the arm that goes first on any machine
with a JDK. I replaced both with `false` and made the fallthrough `return 99`;
all five tests still passed.
Could `shim_bin jar` and `shim_bin jar 1` cover the success and invalid-JAR
paths here?
##########
tools/check-license.sh:
##########
@@ -20,41 +20,83 @@
# 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
Review Comment:
`jar tf` reads the central directory, while `unzip -t` verifies the CRC of
every entry, so the two are not equivalent checks. Measured with JDK 17's
`jar`: a JAR with 32 bytes overwritten inside the entry data but an intact
directory gives `jar tf` 0 and `unzip -tq` 2. Truncation is the realistic
download failure and fails both, so this may never bite in practice. `main`
validated with `unzip -tq` only, though, so preferring `jar` makes the default
path on any JDK-equipped machine the weaker of the two.
Was the ordering picked for availability, or is there a reason to prefer
`jar` on strictness grounds?
##########
tools/check-license.sh:
##########
@@ -20,41 +20,83 @@
# 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"
+ downloaded=false
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
+ downloaded=true
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 --silent --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 --no-verbose --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
+ if [ "$downloaded" = true ]; then
+ rm -f "$JAR"
+ printf "Cannot validate the downloaded Apache RAT JAR: install jar or
unzip.\n" >&2
+ return 1
Review Comment:
Only the cached half of the new split has a test. I gutted this branch,
dropping the `rm -f "$JAR"` and returning 0 instead of 1, and all five tests
stayed green, so nothing holds the fail-closed behavior in place. The shims
already in the file reach it: test 5's curl stub to produce the JAR, plus
`shim_bin_missing jar` and `shim_bin_missing unzip`.
Worth pinning down while the harness is fresh?
##########
tools/check-license.sh:
##########
@@ -20,41 +20,83 @@
# 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"
+ downloaded=false
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
+ downloaded=true
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 --silent --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 --no-verbose --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=$?
Review Comment:
nit: `downloaded` (line 44) and `validation_status` here are the two
variables this PR adds to `acquire_rat_jar`, and neither is scoped, unlike
`jar_cmd` in `validate_rat_jar`. Should these two be `local` as well?
##########
tools/test/unit/check_license.bats:
##########
@@ -0,0 +1,95 @@
+#!/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.
+################################################################################
+
+setup() {
+ load '../helpers/shim'
+ shim_setup
+ CHECK_LICENSE_SOURCE_ONLY=1
+ source "${BATS_TEST_DIRNAME}/../../check-license.sh"
+ unset CHECK_LICENSE_SOURCE_ONLY
+
+
+ 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" -eq 0 ]
+ [[ "$output" == *"Warning: cannot validate cached Apache RAT JAR"* ]]
+ [ -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_script curl 'prev=""; for arg in "$@"; do [[ "$prev" ==
"--output" ]] && : > "$arg"; prev="$arg"; done; exit 22'
+
+ run acquire_rat_jar
+
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"Failed to download Apache RAT"* ]]
+ [ ! -e "${rat_jar}.part" ]
+ [ ! -e "$rat_jar" ]
+}
+
+@test "downloads with curl safety flags and validates the result" {
+ shim_bin_script curl 'while [[ "$1" != "--output" ]]; do shift; done;
touch "$2"'
Review Comment:
This stub has no bound on the loop, so if `--output` is ever absent from
argv, `shift` on an empty list returns non-zero without ending the loop and the
stub spins. I checked by switching the script to curl's equally valid `-o`
form: the suite stops after test 4 and is still going 20 seconds later, so a
regression here costs a CI timeout rather than a red test.
Would the single-pass form you used in test 4 work here too?
--
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]
