Add a shellcheck tool for Review Bot

Review Request #11246 — Created Oct. 26, 2020 and submitted

Information

ReviewBot
master

Reviewers

This adds support for shellcheck through Review Bot. ShellCheck is a
shell script static analysis tool that gives warnings and suggestions
for bash/sh shell scripts.

Configured the tool and ran it manually.

Summary ID Author
Add a shellcheck tool for Review Bot.
This adds support for shellcheck through Review Bot. ShellCheck is a shell script static analysis tool that gives warnings and suggestions for bash/sh shell scripts. Testing done: Configured the tool and ran it manually. Reviewed at https://reviews.reviewboard.org/r/11246/
e093d80e68c8beada38b5d29dd7a8190025521ce ceciliaccwei

Description From Last Updated

I noticed that in files where I saw import logging, I also noticed that this was included: logger = logging.getLogger(__name__) …

jblazusijblazusi

I believe there should be a trailing ',' here.

jblazusijblazusi

I feel like a lot of shell scripts may not have an extension at all. Maybe instead we should look …

daviddavid

Let's say "Using a POSIX-sh compatible shell"

daviddavid

Please add a period to the end of this comment.

daviddavid

A couple comments here: Since we're using regex.search, we shouldn't need to strip the end of the line. Please add …

daviddavid

There is an extra line here.

keanwengkeanweng

I noticed that this call to super() is not compatible with python 2.7, because it expects parameters. I believe that …

jblazusijblazusi

Similarily to other reviews, this execute function should be placed inside of a try/except.

jblazusijblazusi
jblazusi
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 1)
     
     
    Show all issues

    I noticed that in files where I saw import logging, I also noticed that this was included:

    logger = logging.getLogger(__name__)

    In order to name the logger, so that it is easier to track errors, later on.

  3. bot/setup.py (Diff revision 1)
     
     
    Show all issues

    I believe there should be a trailing ',' here.

  4. 
      
ceciliawei
  1. 
      
  2. 
      
ceciliawei
ceciliawei
david
  1. This is looking excellent!

  2. bot/reviewbot/tools/shellcheck.py (Diff revision 3)
     
     
    Show all issues

    I feel like a lot of shell scripts may not have an extension at all. Maybe instead we should look at the first line to see if there's a shebang with a supported shell?

    1. I did not realize this. Thanks for pointing out!
      I searched up on the shebang, looks like the ones below are the most common used in shell scripts:

      #!/bin/sh – Execute the file using the Bourne shell, or a compatible shell, assumed to be in the /bin directory
      #!/bin/bash – Execute the file using the Bash shell
      #!/usr/bin/env bash - Using the env utility
      #!/usr/bin/env sh - Using the env utility
      

      Currently I'm thinking of using these as the patterns to match the shebang. Another thing is that ShellCheck supports dash and ksh in addition to sh and bash. But I am not sure how commonly are those two being used in industries. Do you have any insight on if we should support those two as well?

    2. In my experience, dash is sometimes used by people for their interactive terminals but I've never actually seen a script written in it. ksh used to be quite common, especially on BSD-based systems, but it's been a long time since it was popular and I suspect there aren't a lot of people writing scripts in it these days. Let's just do bash/sh for now, and it'll be easy enough to add dash and ksh later if people ask for them.

  3. 
      
ceciliawei
david
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     
    Show all issues

    Let's say "Using a POSIX-sh compatible shell"

  3. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     
    Show all issues

    Please add a period to the end of this comment.

  4. bot/reviewbot/tools/shellcheck.py (Diff revision 4)
     
     
     
    Show all issues

    A couple comments here:

    1. Since we're using regex.search, we shouldn't need to strip the end of the line.
    2. Please add a blank line between these two (so there's some space above the if block)
    3. We can just use self.shebang_regex instead of first assigning it to a local variable.
  5. 
      
ceciliawei
keanweng
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 5)
     
     
    Show all issues

    There is an extra line here.

    1. Thanks for the review! This is actually the style used in all the ReviewBot tools.

  3. 
      
david
  1. Ship It!
  2. 
      
jblazusi
  1. Ran it manually and it worked great, just one minor issue when used against python 2.7

  2. bot/reviewbot/tools/shellcheck.py (Diff revision 5)
     
     
    Show all issues

    I noticed that this call to super() is not compatible with python 2.7, because it expects parameters. I believe that in order for this to be compatible with python 2.7 it should be super(ShellCheckTool, self).__init__(). I saw that a similar approach was being done in the JSHint tool.

    1. Thanks for catching that. I didn't realize if I do setup develop for python2.7 first and then 3.x versions, the tools will be ran in the last version that got setup. The syntax has now been updated to fit both python2/3.

  3. 
      
ceciliawei
ceciliawei
  1. 
      
  2. 
      
ceciliawei
  1. 
      
  2. 
      
jblazusi
  1. Ship It!
  2. 
      
jblazusi
  1. 
      
  2. bot/reviewbot/tools/shellcheck.py (Diff revision 6)
     
     
    Show all issues

    Similarily to other reviews, this execute function should be placed inside of a try/except.

  3. 
      
ceciliawei
david
  1. Ship It!
  2. 
      
ceciliawei
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (17b2b46)