-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: add shellcheck linter #6428
Conversation
Signed-off-by: kwanhur <huang_hua2012@163.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, let's add #6320 to this PR.
- name: Shellcheck code | ||
run: | | ||
scversion="latest" | ||
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv | |
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv |
Why use -J
here ?
Can it replace with -z
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's format is tar.xz
not tar.gz
-J
filter the archive through xz-z
filter the archive through gzip
scversion="latest" | ||
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv | ||
cp -av "shellcheck-${scversion}/shellcheck" /usr/local/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use apt
to install spellcheck
?
like
apt install -y shellcheck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way is ok to install shellcheck
, but I'm not sure its repository updates to latest version.
- latest version may add new check rules and it covers the before.
- different developers use different version, the latest covers them.
The package must uniformly install from repo unless no supported, I'll improve installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let keep it this way.
Thanks for replying.
Had linked to here. |
I mean squashing the modification in #6320 to this PR. It is bad to split a tiny change into multiple PRs. And if you want to solve more shellcheck warnings, please do it in this PR. Don't bombard with other shellcheck relative PR anymore. |
mentioned PRs differ goal:
I think they're should stay with two, one PR reaches one goal. Of cause, still need they squash into one, I'll take it. |
Signed-off-by: kwanhur <huang_hua2012@163.com>
Signed-off-by: kwanhur <huang_hua2012@163.com>
Anyway, they had been done. |
ci/common.sh
Outdated
@@ -15,6 +15,8 @@ | |||
# limitations under the License. | |||
# | |||
|
|||
# shellcheck disable=SC2148 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is quite strange to add a warning and disable it later. Let's remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SC2148(Add a shebang) rule has enabled, remove it ci code-lint
will run failed.
The key point is that, all the scripts have the shebang but ci/common.sh
and t/cli/common.sh
.
disable
in script file, just scope the file, no influences on the other.
To fix the check failure, three solutions to choose:
- enable SC2148 and add shellcheck disable declaration(just like now), test case SC2148.
- enable SC2148 and add a shebang in
common.sh
, but it seems not so suitable, like ci(action): add shellcheck linter #6340 (comment). - disable SC2148, this will not find it when add a script without shebang.
Summary, solution 1 could be the better.
Hope your voice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but why use
enable=SC2148
Could we keep it disabled? Look like the check is enabled by you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had disable it.
Actually, all the rules enable by default. Here, I mark it obviously.
Signed-off-by: kwanhur <huang_hua2012@163.com>
build (ubuntu-18.04, linux_openresty_1_17, t/plugin) runs failed, it's not related to from the logs, seems that grpc server launch abnormally. |
.github/workflows/code-lint.yml
Outdated
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv | ||
cp -av "shellcheck-${scversion}/shellcheck" /usr/local/bin/ | ||
shellcheck --version | ||
shellcheck **/*.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's limit the scope. We don't want to lint the code from git submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I got the meaning that checkout repo without submodule, so it won't lint the code from git submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's limit the scope. We don't want to lint the code from git submodule.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck **/*.sh
? It is still linting all the shell files in this repo, including the one from submodule...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution 1: not checkout submodule, then this won't lint shell files in submodule
steps:
- name: Checkout code
uses: actions/checkout@v2.4.0
Solution 2: specify shellcheck paths, exclude submodule, so it won't lint in submodule
From this tip, I think you mean the solution 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shellcheck **/*.sh
? It is still linting all the shell files in this repo, including the one from submodule...
Specify file and dirs to lint had been done.
Signed-off-by: kwanhur <huang_hua2012@163.com>
Signed-off-by: kwanhur <huang_hua2012@163.com>
Signed-off-by: kwanhur <huang_hua2012@163.com>
.github/workflows/code-lint.yml
Outdated
wget -O- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv | ||
cp -av "shellcheck-${scversion}/shellcheck" /usr/local/bin/ | ||
shellcheck --version | ||
shellcheck benchmark/*.sh bin/apisix ci/*.sh ci/pod/nacos/healthcheck/*.sh t/cli/*.sh t/plugin/grpc-web/*.sh utils/*.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to use git ls-files | grep "\.sh$" | xargs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's better. xargs -t shellcheck
echo the command to be executed, it'll show which files to check.
Signed-off-by: kwanhur <huang_hua2012@163.com>
What this PR does / why we need it:
use ShellCheck to analysis shell scripts and gives warnings and suggestions.
intention: import shell script's quality
test case SC2148
.shellcheckrc
to enable/disable shellcheck code(equals rule), content now comes from existed shell scripts.#6320 (comment)
Pre-submission checklist: