[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts

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

fgerlits commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r603462998



##########
File path: bstrp_functions.sh
##########
@@ -75,30 +75,31 @@ set_incompatible_with(){
 }
 
 print_multi_option_status(){
-  feature="$1"
   feature_status=${!1}
   declare -a VAR_OPTS=()
 
-  declare VAR_OPTS=$1_OPTIONS[@]
-  VAR_OPTS=$1_OPTIONS[@]
+  declare VAR_OPTS=("$1_OPTIONS[@]")
+  VAR_OPTS=("$1_OPTIONS[@]")
 
   for option in "${!VAR_OPTS}" ; do
     if [ "${option}" = "$feature_status" ]; then
+      # shellcheck disable=SC2059
     	printf "${RED}"
     fi
+    # shellcheck disable=SC2059
     printf "${option}"
+    # shellcheck disable=SC2059
     printf "${NO_COLOR} "

Review comment:
       This could be rewritten as
   ```suggestion
       if [ "${option}" = "$feature_status" ]; then
       	printf "%b%s%b " "${RED}" "${option}" "${NO_COLOR}"
       else
       	printf "%s " "${option}"
       fi
   ```
   which I think would be more readable.

##########
File path: centos.sh
##########
@@ -47,20 +46,20 @@ install_bison() {
     if [ "$OS_MAJOR" = "6" ]; then
         BISON_INSTALLED="false"
         if [ -x "$(command -v bison)" ]; then
-            BISON_VERSION=`bison --version | head -n 1 | awk '{print $4}'`
-            BISON_MAJOR=`echo $BISON_VERSION | cut -d. -f1`
+            BISON_VERSION=$(bison --version | head -n 1 | awk '{print $4}')
+            BISON_MAJOR=$(echo "$BISON_VERSION" | cut -d. -f1)
             if (( BISON_MAJOR >= 3 )); then
                 BISON_INSTALLED="true"
             fi
         fi
         if [ "$BISON_INSTALLED" = "false" ]; then
             wget https://ftp.gnu.org/gnu/bison/bison-3.0.4.tar.xz
             tar xvf bison-3.0.4.tar.xz
-            pushd bison-3.0.4
+            pushd bison-3.0.4 || return

Review comment:
       Elsewhere you used `exit 1`, which I think is better.  If we silently return, we are likely to get a cryptic error message later.

##########
File path: .github/workflows/ci.yml
##########
@@ -159,7 +159,7 @@ jobs:
           sudo apt install -y ccache libfl-dev libpcap-dev libboost-all-dev
           echo "PATH=/usr/lib/ccache:$PATH" >> $GITHUB_ENV
       - id: build
-        run: ./bootstrap.sh -e -t && cd build  && cmake -DUSE_SHARED_LIBS= -DCMAKE_BUILD_TYPE=Release -DENABLE_BUSTACHE=ON -DENABLE_SQLITE=ON -DENABLE_PCAP=ON -DSTRICT_GSL_CHECKS=AUDIT -DFAIL_ON_WARNINGS=ON .. && make -j4 VERBOSE=1  && make test ARGS="--timeout 300 -j2 --output-on-failure"
+        run: ./bootstrap.sh -e -t && cd build  && cmake -DUSE_SHARED_LIBS= -DCMAKE_BUILD_TYPE=Release -DENABLE_BUSTACHE=ON -DENABLE_SQLITE=ON -DENABLE_PCAP=ON -DSTRICT_GSL_CHECKS=AUDIT -DFAIL_ON_WARNINGS=ON .. && make -j4 VERBOSE=1  && make test ARGS="--timeout 300 -j2 --output-on-failure" && make shellcheck

Review comment:
       I would make this a separate `step` rather than adding it to the end of the `build` step.  That would make it easier to find the shellcheck output in the Checks window.




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