Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ci: add shellcheck linter #6428

Merged
merged 8 commits into from
Mar 4, 2022
Merged

ci: add shellcheck linter #6428

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c734b16
ci: add shellcheck linter
kwanhur Feb 22, 2022
56b6700
ci(apisix test): grep search [error] from file error.log
kwanhur Feb 14, 2022
8f27197
ci(apisix test): abandon shellcheck directive
kwanhur Feb 15, 2022
fa549ac
ci(shellcheck): disable SC2148 add a shebang
kwanhur Feb 28, 2022
38c7d0e
ci(shellcheck): no lint on submodule's codes
kwanhur Mar 1, 2022
7c935af
ci(shellcheck): specify file and directories
kwanhur Mar 2, 2022
92f3d06
ci(shellcheck): specify accurate directory pattern
kwanhur Mar 2, 2022
1c191ca
ci(shellcheck): git list files
kwanhur Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/code-lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,20 @@ jobs:
. ./ci/common.sh
export_or_prefix
make lint

sc-lint:
runs-on: ubuntu-latest
timeout-minutes: 5
steps:
- name: Checkout code
uses: actions/checkout@v2.4.0
with:
submodules: true

- 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 ?

Copy link
Contributor Author

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

cp -av "shellcheck-${scversion}/shellcheck" /usr/local/bin/
Comment on lines +41 to +43
Copy link
Member

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

Copy link
Contributor Author

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.

  1. latest version may add new check rules and it covers the before.
  2. different developers use different version, the latest covers them.

The package must uniformly install from repo unless no supported, I'll improve installation.

Copy link
Member

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.

shellcheck --version
shellcheck **/*.sh
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

28 changes: 28 additions & 0 deletions .shellcheckrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#
# 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.
#

# wiki link pattern https://github.com/koalaman/shellcheck/wiki/{code}
# like SC2148, links to https://github.com/koalaman/shellcheck/wiki/SC2148

#SC2148 Add a shebang
enable=SC2148

disable=SC1083,SC1091
disable=SC2002,SC2004,SC2006,SC2009,SC2010,SC2016,SC2024,SC2027,SC2034,SC2046,SC2048,SC2059
disable=SC2068,SC2076,SC2086,SC2089,SC2090
disable=SC2103,SC2126,SC2143,SC2155,SC2157,SC2164,SC2181
disable=SC2215,SC2216,SC2251,SC2260,SC3037
2 changes: 2 additions & 0 deletions ci/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
# limitations under the License.
#

# shellcheck disable=SC2148
Copy link
Member

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.

Copy link
Contributor Author

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:

  1. enable SC2148 and add shellcheck disable declaration(just like now), test case SC2148.
  2. enable SC2148 and add a shebang in common.sh, but it seems not so suitable, like ci(action): add shellcheck linter #6340 (comment).
  3. disable SC2148, this will not find it when add a script without shebang.

Summary, solution 1 could be the better.
Hope your voice.

Copy link
Member

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

in https://github.com/apache/apisix/pull/6428/files#diff-c71502ae2fb6ee770263dbf93031c6bb1549ef6b2970b862daf24d01a1ded277R22?

Could we keep it disabled? Look like the check is enabled by you.

Copy link
Contributor Author

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.


set -ex

export_or_prefix() {
Expand Down
2 changes: 1 addition & 1 deletion ci/linux_apisix_current_luarocks_runner.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ script() {
sudo PATH=$PATH apisix start
sudo PATH=$PATH apisix stop

cat /usr/local/apisix/logs/error.log | grep '\[error\]' > /tmp/error.log | true
grep '\[error\]' /usr/local/apisix/logs/error.log > /tmp/error.log | true
if [ -s /tmp/error.log ]; then
echo "=====found error log====="
cat /usr/local/apisix/logs/error.log
Expand Down
2 changes: 1 addition & 1 deletion ci/linux_apisix_master_luarocks_runner.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ script() {
# apisix cli test
# todo: need a more stable way

cat /usr/local/apisix/logs/error.log | grep '\[error\]' > /tmp/error.log | true
grep '\[error\]' /usr/local/apisix/logs/error.log > /tmp/error.log | true
if [ -s /tmp/error.log ]; then
echo "=====found error log====="
cat /usr/local/apisix/logs/error.log
Expand Down
2 changes: 2 additions & 0 deletions t/cli/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
# The 'apisix' command is a command in the /usr/local/apisix,
# and the configuration file for the operation is in the /usr/local/apisix/conf

# shellcheck disable=SC2148

set -ex

check_failure() {
Expand Down