[GitHub] [ozone] adoroszlai commented on a change in pull request #1778: HDDS-2402. Adapt hadolint check to improved CI framework

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #1778:
URL: https://github.com/apache/ozone/pull/1778#discussion_r556508057



##########
File path: hadoop-ozone/dev-support/checks/hadolint.sh
##########
@@ -17,25 +17,30 @@ DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
 cd "$DIR/../../.." || exit 1
 REPO_DIR="$DIR/../../.."
 
-ERROR=0
-
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/hadolint"}
+mkdir -p "$REPORT_DIR"
+REPORT_FILE="$REPORT_DIR/summary.txt"
 
 for Dockerfile in $(find "$REPO_DIR/hadoop-ozone" "$REPO_DIR/hadoop-hdds" -name Dockerfile | sort); do
-  echo "Checking $Dockerfile"
+  echo "Checking $Dockerfile" | tee -a "$REPORT_FILE"

Review comment:
       This would cause exit code 1, because even without any errors, at least `Checking ...` would be present in the report file, hence `-s` evaluates to true.  I think we can remove this line, as hadolint's error messages include the input file's path.

##########
File path: hadoop-ozone/dev-support/checks/hadolint.sh
##########
@@ -17,25 +17,30 @@ DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
 cd "$DIR/../../.." || exit 1
 REPO_DIR="$DIR/../../.."
 
-ERROR=0
-
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/hadolint"}
+mkdir -p "$REPORT_DIR"
+REPORT_FILE="$REPORT_DIR/summary.txt"
 
 for Dockerfile in $(find "$REPO_DIR/hadoop-ozone" "$REPO_DIR/hadoop-hdds" -name Dockerfile | sort); do

Review comment:
       I suggest the following improvement:
   
   ```suggestion
   for Dockerfile in $(find hadoop-ozone hadoop-hdds -name Dockerfile | sort); do
   ```
   
   This would provide cleaner output, eg.:
   
   ```
   hadoop-ozone/dev-support/docker/Dockerfile
   ```
   
   instead of
   
   ```
   /opt/ozone/src/ozone/hadoop-ozone/dev-support/checks/../../../hadoop-ozone/dev-support/docker/Dockerfile
   ```

##########
File path: hadoop-ozone/dev-support/checks/hadolint.sh
##########
@@ -17,25 +17,30 @@ DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
 cd "$DIR/../../.." || exit 1
 REPO_DIR="$DIR/../../.."
 
-ERROR=0
-
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/hadolint"}
+mkdir -p "$REPORT_DIR"
+REPORT_FILE="$REPORT_DIR/summary.txt"
 
 for Dockerfile in $(find "$REPO_DIR/hadoop-ozone" "$REPO_DIR/hadoop-hdds" -name Dockerfile | sort); do
-  echo "Checking $Dockerfile"
+  echo "Checking $Dockerfile" | tee -a "$REPORT_FILE"
 
   result=$( hadolint $Dockerfile )
   if [ ! -z "$result" ]
   then
-    echo "$result"
-    echo ""
-    ERROR=1
+    echo "$result" | tee -a "$REPORT_FILE"
+    echo "" | tee -a "$REPORT_FILE"
   fi
 done
 
-if [ "$ERROR" = 1 ]
+cat "$REPORT_FILE" |  egrep -v '^$|^Checking' | wc -l | awk '{print $1}'> "$REPORT_DIR/failures"
+
+if [ -s "${REPORT_FILE}" ]
 then
-  echo ""
-  echo ""
-  echo "Hadolint errors were found. Exit code: 1."
-  exit 1
+  echo "" | tee -a "$REPORT_FILE"
+  echo "" | tee -a "$REPORT_FILE"
+  echo "Hadolint errors were found. Exit code: 1." | tee -a "$REPORT_FILE"
+fi
+
+if [[ -s "${REPORT_FILE}" ]]; then

Review comment:
       Nit: these two `if`s can be collapsed (no need to check the condition twice).




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org