You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/10 15:20:34 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
lordgamez opened a new pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604677564
##########
File path: README.md
##########
@@ -591,6 +591,10 @@ We welcome all contributions to Apache MiNiFi. To make development easier, we've
the linter for the Google Style guide. Google provides an Eclipse formatter for their style
guide. It is located [here](https://github.com/google/styleguide/blob/gh-pages/eclipse-cpp-google-style.xml).
New contributions are expected to follow the Google style guide when it is reasonable.
+
Review comment:
Added more information on disabling an issue and link to wiki page in 5a000df55489d9cb910dd5897c2d506b5847bb02
--
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
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604024246
##########
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:
Great suggestion! Updated in 4650e8b54fcab4ef1c6a709a2cfdc21eadb2903b
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards removed a comment on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards removed a comment on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810407665
for example ( this may be overkill obviously), I have this in my headers:
```bash
# This script is verified by shellcode. Any suppressions shall be listed here
# with the reason for suppression.
#
# | shellcode error number | reason for suppression |
# |---------------------------------------------------------------------------|
# example "because"
#
################################################################################
```
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604061796
##########
File path: run_shellcheck.sh
##########
@@ -0,0 +1,6 @@
+#!/bin/bash
Review comment:
This file requires Apache Lic. Headers
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604085084
##########
File path: run_shellcheck.sh
##########
@@ -0,0 +1,6 @@
+#!/bin/bash
Review comment:
Updated in e11796030ee7b86b3d257ddc7e528c708ca411ed
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810407665
for example ( this may be overkill obviously), I have this in my headers:
```bash
# This script is varified by shellcode. Any suppressions shall be listed here
# with the reason for suppression.
#
# | shellcode error number | reason for suppression |
# |---------------------------------------------------------------------------|
# example "because"
#
################################################################################
```
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604252231
##########
File path: README.md
##########
@@ -591,6 +591,10 @@ We welcome all contributions to Apache MiNiFi. To make development easier, we've
the linter for the Google Style guide. Google provides an Eclipse formatter for their style
guide. It is located [here](https://github.com/google/styleguide/blob/gh-pages/eclipse-cpp-google-style.xml).
New contributions are expected to follow the Google style guide when it is reasonable.
+
Review comment:
The only thing I would think about adding here, that I've had to add myself in my shellchecked systems, is some policy on disabling.
"If a shellcheck code is disabled explicitly by a script, that should be commented with an explanation at that point and subject to review."
That way it is clear to the reviewer and future people why something is the way it is. This assumes that there will be instances where *some* tolerance is required.
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards edited a comment on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards edited a comment on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810407665
for example ( this may be overkill obviously), I have this in my headers:
```bash
# This script is verified by shellcode. Any suppressions shall be listed here
# with the reason for suppression.
#
# | shellcode error number | reason for suppression |
# |---------------------------------------------------------------------------|
# example "because"
#
################################################################################
```
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604024850
##########
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:
Updated in 4650e8b54fcab4ef1c6a709a2cfdc21eadb2903b.
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810260499
Should this be documented somewhere? The readme?
"minifi cpp utilizes shellcheck (through the make shellcheck target ) to ............. Failures are expected to be .........."
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by 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
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810307410
> Should this be documented somewhere? The readme?
> "minifi cpp utilizes shellcheck (through the make shellcheck target ) to ............. Failures are expected to be .........."
That's a good point. I added some brief information to the Contribution section of the README.md similarly to the information about `cpplint`. Although I don't want to go into the specifics of the verification process as it is not part of the README currently.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604024746
##########
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:
Added different exit codes for errors within a file in all occurrences in 4650e8b54fcab4ef1c6a709a2cfdc21eadb2903b.
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on a change in pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#discussion_r604258192
##########
File path: README.md
##########
@@ -591,6 +591,10 @@ We welcome all contributions to Apache MiNiFi. To make development easier, we've
the linter for the Google Style guide. Google provides an Eclipse formatter for their style
guide. It is located [here](https://github.com/google/styleguide/blob/gh-pages/eclipse-cpp-google-style.xml).
New contributions are expected to follow the Google style guide when it is reasonable.
+
Review comment:
for example ( this may be overkill obviously), I have this in my headers:
```bash
# This script is verified by shellcode. Any suppressions shall be listed here
# with the reason for suppression.
#
# | shellcode error number | reason for suppression |
# |---------------------------------------------------------------------------|
# example "because"
#
################################################################################
```
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-810400980
Super, the issue is "If I submit a PR, or I'm a reviewer, what does shellcheck mean?" Can my PR be rejected until shellcheck passes etc etc
--
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
[GitHub] [nifi-minifi-cpp] ottobackwards commented on pull request #1026: MINIFICPP-1524 Add shellcheck integration for linting shell scripts
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #1026:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1026#issuecomment-819128870
@lordgamez thanks for this contribution!
--
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