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.

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

[Travis CI] use "ShellCheck" to validate the shell scripts in xCAT commits #4610

Open
immarvin opened this issue Jan 8, 2018 · 13 comments
Open

Comments

@immarvin
Copy link
Contributor

immarvin commented Jan 8, 2018

ShellCheck is a GPLv3 tool that gives warnings and suggestions for bash/sh shell scripts, we can leverage it to impose a strict check on the shell scripts in xCAT

From https://github.com/koalaman/shellcheck:
The goals of ShellCheck are

  • To point out and clarify typical beginner's syntax issues that cause a shell to give cryptic error messages.

  • To point out and clarify typical intermediate level semantic problems that cause a shell to behave strangely and counter-intuitively.

  • To point out subtle caveats, corner cases and pitfalls that may cause an advanced user's otherwise working script to fail under future circumstances.
    Travis CI Setup

If you want to use ShellCheck in Travis CI, you can most easily install it via apt:

language: bash
addons:
  apt:
    sources:
    - debian-sid    # Grab ShellCheck from the Debian repo
    packages:
    - shellcheck
@daniceexi
Copy link
Contributor

👍 Good to see the action that we are looking for the ways to improve ourselves.

@hu-weihua
Copy link

@immarvin, thanks for your information. It is very helpful for test.

@samveen
Copy link
Member

samveen commented Jan 9, 2018

Packages for shellcheck are also available for RH/Centos from EPEL (named ShellCheck).

@samveen
Copy link
Member

samveen commented Jan 9, 2018

@immarvin @hu-weihua A lot of the scripts are expected to run on very diverse systems. For those systems that provide bash, it may be good to create a matrix of expected bash versions against the base systems as a lot of shell features are introduced by newer major Bash versions.

In this particular case, I see a script that expects to work on both AIX and linux. However I am unsure of what bash version is available on AIX, which means that I cannot make changes to it because I might be introducing gotchas depending on the version of bash available on AIX.

@immarvin
Copy link
Contributor Author

immarvin commented Jan 9, 2018

hi @samveen , thanks for the explanation.
I remember that AIX does not ship bash by default, postscripts for AIX are supposed to be run with ksh(or sh linked to ksh). And now we do not cover AIX verification in each xcat release.

So I think you do not need to consider AIX support while modifying the postscripts unless:

  1. the postscript for AIX only, i.e, the postscripts with the name aix* or with she-bang #!/bin/ksh
  2. the code slices for AIX only, i.e, the code in conditional branches like if [ $OS == "AIX" ]; then

For the scripts that are run with bash, the bash version is determined by the bash shipped in linux distributions we support, we suppose the postscripts can be run on all the linux distributions which are announced to be supported in the xcat release notes

@samveen
Copy link
Member

samveen commented Jan 9, 2018

@immarvin For any scripts with AIX specific code paths inside common scripts will still need non-bash specific code in the common areas. This makes the script syntax fixes a lot harder.

On a side note, Is AIX still supported, and how long will it remain as a supported system?

@immarvin
Copy link
Contributor Author

hi @samveen , checked this with team, for xCAT releases after 2.12, we do not announce support for AIX any longer

@samveen
Copy link
Member

samveen commented Jan 10, 2018

Can I safely expect Bash >= 4.0 as a minimum available on all platforms where xCAT is expecting to run? If it is not explicitly mentioned, can this be added as an explicit requirement for xCAT servers and for the OS Images for all provisioned nodes?

@immarvin
Copy link
Contributor Author

hi @samveen , sles11.4 is still in the support list of xCAT 2.13.x and sles11.4 ships bash 3.x

@samveen
Copy link
Member

samveen commented Jan 11, 2018

@immarvin I am working on shellchecking the scripts inside xCAT-genesis-scripts/ which are confirmed to run on Centos 7.4 and therefore bash >= 4.0 . The rest can proceed on a case by case basis.

@hu-weihua
Copy link

Due to xCAT team will use IBM internal github to manage task. the related task is https://github.ibm.com/xcat2/task_management/issues/177.

@gurevichmark
Copy link
Contributor

@samveen Is your plan to go through all xCAT scripts and make them pass shellcheck ?
Is your plan after that to add shellcheck to TravisCI to run it for each PR ?

@samveen
Copy link
Member

samveen commented Jul 9, 2019

@gurevichmark Yes, I was planning on getting as much of the shell portion of the project shellchecked as possible.
As to adding CI checks, I hadn't really thought about it, but I can take a look and see if I can help with that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants