search

firecracker-microvm / firecracker

Secure and fast microVMs for serverless computing.
http://firecracker-microvm.io
Apache License 2.0
25.09k stars 1.76k forks source link

Add bash linter integration test #2903

Closed alindima closed 11 months ago

alindima commented 2 years ago

Install shellcheck (or a similar checker) inside devctr and add a python integration test that runs it on the shell scripts we have in Firecracker, like tools/devtool and other .sh files

austinvazquez commented 2 years ago

@alindima, I would like to own this issue. IMO, the change boils down to

  1. Adding shellcheck to devctr.
  2. Resolving issues found by static analysis tool.
  3. Adding a build integration test for verification.
  4. Adding a devtool command to run shellcheck.

The first task may require collaboration with yourself or another maintainer. The following patch should be enough to add shellcheck via package manager.

diff --git a/tools/devctr/Dockerfile.aarch64 b/tools/devctr/Dockerfile.aarch64
index 94fcf5fa..114a45fa 100644
--- a/tools/devctr/Dockerfile.aarch64
+++ b/tools/devctr/Dockerfile.aarch64
@@ -58,6 +58,7 @@ RUN apt-get update \
         bc \
         flex \
         bison \
+        shellcheck \
     && python3 -m pip install \
         setuptools \
         setuptools_rust \
diff --git a/tools/devctr/Dockerfile.x86_64 b/tools/devctr/Dockerfile.x86_64
index 1e58b406..a8929fa2 100644
--- a/tools/devctr/Dockerfile.x86_64
+++ b/tools/devctr/Dockerfile.x86_64
@@ -58,6 +58,7 @@ RUN apt-get update \
         bc \
         flex \
         bison \
+        shellcheck \
     && python3 -m pip install \
         setuptools \
         wheel \

Using ubuntu:18.04, we get access to version 0.4.6-1 which is recent enough that I don't think it warrants installing from source.

Thoughts?

dianpopa commented 2 years ago

Hi @austinvazquez

Very happy you want to help with this issue. The steps you propose sound good to me. I will help you with pushing the docker image once the PR is up.

austinvazquez commented 2 years ago

@dianpopa thanks for the feedback. I'll begin looking into the changes and open a PR soon.

roypat commented 11 months ago

We've discussed this with the team today and decided against adding shellcheck to our CI. This is mainly due to having bad experiences with its false-positive rates, that are similar to markdownlint, which already creates a lot of noise for us.