Add shell scripts CI validations
Open, MediumPublic
Actions

Assigned To

None

Authored By

	Volans
	Oct 18 2016, 8:25 AM

Description

As discussed in the Operations offsite we'd like to add some CI checks for shell scripts, probably non voting at the start given the current status of the repo.

The first two tools that we could add are:

ShellCheck (packaged for Jessie).
checkbashisms (from the devscripts Debian package). See T95064 for the current status of the operations/puppet repo as an example.

And of course investigate to see if there are more worth adding.
We should also agree on a policy for 3rd party scripts that were imported as is in our repositories:

Keeping as is and excluding them from the checks + optional reporting it upstream and wait for a new version
Modifying them with a clear Changelog entry + optional reporting it upstread with a pull request

Details

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
varnish: add .sh suffix to shell scripts	operations/puppet	production	+4 -4
icinga/CI: give all shell scripts a file extension	operations/puppet	production	+8 -16
toollabs/CI: give banner scripts an .sh extension	operations/puppet	production	+5 -22
installserver/CI: give shell scripts a file extension	operations/puppet	production	+3 -3

Customize query in gerrit

Related Objects

Mentioned In: T346163: Implement a template for Shell scripting testing
T200984: Stop introducing new code expanded from erb templates
T259586: Add CI to mediawiki/tools/train-dev
T245266: Add a shell script linter
T175906: Add Shell linting to heritage repo
T95064: Bashisms in various /bin/sh scripts
Mentioned Here: T254480: Shell/Python/other scripts should not be generated by ERB files; dynamic parts should be a simple ERB config file
T144169: Flake8 for python files without extension in puppet repo
T95064: Bashisms in various /bin/sh scripts

Event Timeline

Volans created this task.Oct 18 2016, 8:25 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2016, 8:25 AM

Volans mentioned this in T95064: Bashisms in various /bin/sh scripts.Oct 18 2016, 8:27 AM

• ema triaged this task as Medium priority.Oct 18 2016, 8:33 AM

• ema subscribed.

hashar updated the task description. (Show Details)Oct 24 2016, 9:45 AM

hashar moved this task from Backlog to Repo setup on the Continuous-Integration-Config board.

My editor uses both and they are quite useful to avoid mistakes and preventing corner cases issues.

The main challenge though is neither support file tree traversal to find scripts, if we had a policy to have all shell scripts suffixed with .sh and have puppet to install them prefixless, that would help. Then one would just:

find . -name '*.sh' -exec checkbashisms {} +
find . -name '*.sh' -exec shellcheck {} +

Then have a wrapper to catch the non zero exit codes and report.

Self note: for Jenkins to nicely display result:

checkbashisms has a TAP wrapper: checkbashisms_tap
shellcheck supports checkstyle output

For the 3rd party scripts, I would just ignore them. Potentially by introducing a /.shellintignore file that would have patterns we can then pass to find using something like

find . -prune (cat /shellintignore) ...

Finally we will need an entry point in puppet.git to easily run the command (maybe reuse rake) or we craft a new Jenkins job that only reacts when .sh files are altered. I dont mind either way, the command being in the puppet.git repo making it easier for you to adjust and let one reproduce the CI run from their local machine (eg one would run rake lint:shell).

In T148494#2737503, @hashar wrote:

Finally we will need an entry point in puppet.git to easily run the command (maybe reuse rake) or we craft a new Jenkins job that only reacts when .sh files are altered. I dont mind either way, the command being in the puppet.git repo making it easier for you to adjust and let one reproduce the CI run from their local machine (eg one would run rake lint:shell).

My two cents: agreed the "single command" option is better vs a specialized CI job

Looks like we have about 206 scripts with "bin/bash" in ops/puppet. About 56 are called .sh or .sh.erb. Would we want to rename the other 150 for this?

My two cents: agreed the "single command" option is better vs a specialized CI job

For operations/puppet.git, CI currently relays on a couple entry points:

tox
rake test

We also have a few dedicated jobs, erblint-HEAD pplint-HEAD and puppetlint-strict. I got some patches to integrate them in the rake test command though :]

typos which shells out to a grep command feed with the /typos file.

Should we look at integrating shellcheck/checkbashisms in the Rakefile?

170 files based on gfind . -type f ! -path '*/.git/*' -a ! -name '*.*' -exec file --mime {} +|grep x-shell|cut -d\ -f1

./files/mha/mha_site_switch:
./files/misc/scripts/install_console:
./modules/admin/files/home/ori/.binned/disable-puppet:
./modules/admin/files/home/ori/.hosts/bast1001:
./modules/admin/files/home/ori/.hosts/fluorine:
./modules/admin/files/home/ori/.hosts/puppetmaster1001:
./modules/admin/files/home/ori/.hosts/tin:
./modules/admin/files/home/rush/bin/rsched:
./modules/apache/files/apache-status:
./modules/aptrepo/files/log:
./modules/aptrepo/files/reprepro-ssh-upload:
./modules/archiva/files/archiva-gitfat-link:
./modules/authdns/files/authdns-git-pull:
./modules/authdns/files/authdns-lint:
./modules/authdns/files/authdns-local-update:
./modules/authdns/files/authdns-update:
./modules/backup/files/openldap-post:
./modules/backup/files/openldap-pre:
./modules/base/files/check_sysctl:
./modules/base/files/environment/gen_fingerprints:
./modules/base/files/firewall/check_ferm:
./modules/base/files/puppet/97-last-puppet-run:
./modules/base/files/puppet/puppet-enabled:
./modules/base/files/puppet/puppet-run:
./modules/base/files/puppet/run-no-puppet:
./modules/base/files/resolv/nodnsupdate:
./modules/cassandra/files/cassandra-metrics-collector:
./modules/cassandra/files/nodetool-instance:
./modules/confd/files/check_confd_template:
./modules/deployment/files/git-deploy/utils/submodule-update-server-info:
./modules/deployment/files/git-new-workdir:
./modules/docker/files/setup-docker:
./modules/elasticsearch/files/nagios/check_elasticsearch:
./modules/eventlogging/files/check_eventlogging_jobs:
./modules/eventlogging/files/eventloggingctl:
./modules/graphite/files/carbonctl:
./modules/gridengine/files/collector:
./modules/gridengine/files/gridengine-mailer:
./modules/gridengine/files/mergeconf:
./modules/gridengine/files/tracker:
./modules/haproxy/files/check_haproxy:
./modules/hhvm/files/debug/hhvm-collect-heaps:
./modules/hhvm/files/debug/hhvm-diff-heaps:
./modules/hhvm/files/debug/hhvm-dump-debug:
./modules/hhvm/files/debug/hhvmadm:
./modules/hhvm/files/debug/install-pkg-src:
./modules/icinga/files/check_iostat:
./modules/icinga/files/check_mailman_queue:
./modules/icinga/files/check_wikitech_static:
./modules/icinga/files/icinga-downtime:
./modules/icinga/files/icinga-init:
./modules/icinga/files/submit_check_result:
./modules/install_server/files/autoinstall/scripts/early_command:
./modules/install_server/files/autoinstall/scripts/late_command:
./modules/install_server/files/autoinstall/scripts/partman_early_command:
./modules/ipmi/files/ipmi_mgmt:
./modules/java/files/jheapdump:
./modules/keyholder/files/check_keyholder:
./modules/keyholder/files/keyholder:
./modules/labs_bootstrapvz/files/firstbootrc:
./modules/labs_lvm/files/extend-instance-vol:
./modules/labs_lvm/files/make-instance-vg:
./modules/labs_lvm/files/make-instance-vol:
./modules/labstore/files/block-for-export:
./modules/labstore/files/getent_check:
./modules/labstore/files/nfs-mount-manager:
./modules/labstore/files/set-stripe-cache:
./modules/labstore/files/start-nfs:
./modules/labstore/files/sync-exports:
./modules/ldap/files/modify-ldap-group:
./modules/ldap/files/modify-ldap-user:
./modules/lvs/files/pybal/check-apache:
./modules/mariadb/files/mysqld_safe:
./modules/mediawiki/files/cgroup/cgroup-mediawiki-clean:
./modules/mediawiki/files/maintenance/characterEditStatsTranslate:
./modules/mediawiki/files/maintenance/update-article-count:
./modules/mediawiki/files/maintenance/update-special-pages:
./modules/mediawiki/files/mw-pool:
./modules/mediawiki/files/mwrepl:
./modules/memcached/files/memkeys-snapshot:
./modules/mgmt/files/changepw:
./modules/mgmt/files/getmgmtips:
./modules/mirrors/files/archvsync/bin/ftpsync:
./modules/mirrors/files/check_apt_mirror:
./modules/nagios_common/files/check_commands/check_dsh_groups:
./modules/nagios_common/files/check_commands/check_icinga_config:
./modules/nagios_common/files/check_commands/check_ores_workers:
./modules/nagios_common/files/check_commands/check_prometheus_metric:
./modules/nagios_common/files/check_commands/check_ssl_certfile:
./modules/nagios_common/files/check_commands/check_to_check_nagios_paging:
./modules/nagios_common/files/check_commands/check_wikidata:
./modules/nodepool/files/nodepool-graceful-stop:
./modules/nrpe/files/plugins/check_dpkg:
./modules/puppet/files/self-master-post-receive:
./modules/puppet_compiler/files/compiler-update-facts:
./modules/puppetmaster/files/git/pre-commit:
./modules/puppetmaster/files/git/pre-merge:
./modules/puppetmaster/files/git/pre-rebase:
./modules/puppetmaster/files/git/private/commit-msg-master:
./modules/puppetmaster/files/git/private/post-merge:
./modules/puppetmaster/files/git/private/post-receive:
./modules/puppetmaster/files/git/private/post-receive-master:
./modules/puppetmaster/files/git/private/pre-commit:
./modules/puppetmaster/files/git/private/pre-merge:
./modules/puppetmaster/files/git/private/pre-rebase:
./modules/puppetmaster/files/install-console:
./modules/puppetmaster/files/make-labs-root-password:
./modules/puppetmaster/files/motd/99-obsolete:
./modules/puppetmaster/files/puppet-facts-export:
./modules/puppetmaster/files/wmf-reimage:
./modules/quarry/files/celeryd:
./modules/rcstream/files/rcstreamctl:
./modules/releases/files/deb-upload:
./modules/role/files/analytics_cluster/hadoop/check_hadoop_yarn_node_state:
./modules/role/files/analytics_cluster/hadoop/hdfs-balancer:
./modules/role/files/logging/fatalmonitor:
./modules/role/files/logging/mw-log-cleanup:
./modules/role/files/toollabs/clush/clush:
./modules/salt/files/check_unaccepted_keys:
./modules/scap/files/commit-msg:
./modules/scap/files/dologmsg:
./modules/scap/files/foreachwiki:
./modules/scap/files/foreachwikiindblist:
./modules/scap/files/l10nupdate:
./modules/scap/files/l10nupdate-1:
./modules/scap/files/mwscript:
./modules/scap/files/mwscriptwikiset:
./modules/scap/files/purge-varnish:
./modules/scap/files/scap-master-sync:
./modules/scap/files/set-group-write:
./modules/scap/files/set-group-write2:
./modules/scap/files/sql:
./modules/scap/files/sqldump:
./modules/shinken/files/init:
./modules/sslcert/files/update-ocsp-all:
./modules/statsite/files/statsitectl:
./modules/strongswan/files/ipsec-global:
./modules/swift/files/swift-labs-ring:
./modules/tlsproxy/files/update-ocsp-nginx-hook:
./modules/toollabs/files/40-tools-bastion-banner:
./modules/toollabs/files/40-tools-exechost-banner:
./modules/toollabs/files/40-tools-infrastructure-banner:
./modules/toollabs/files/40-tools-submithost-banner:
./modules/toollabs/files/40-toolsbeta-bastion-banner:
./modules/toollabs/files/40-toolsbeta-exechost-banner:
./modules/toollabs/files/40-toolsbeta-infrastructure-banner:
./modules/toollabs/files/40-toolsbeta-submithost-banner:
./modules/toollabs/files/check-pause-container:
./modules/toollabs/files/exec-manage:
./modules/toollabs/files/gethgrp:
./modules/toollabs/files/jlocal:
./modules/toollabs/files/localuser:
./modules/toollabs/files/maintainers:
./modules/toollabs/files/project-make-access:
./modules/toollabs/files/project-make-shosts:
./modules/toollabs/files/sessionclean:
./modules/toollabs/files/sql:
./modules/torrus/files/torrus-discovery:
./modules/udp2log/files/check_udp2log_log_age:
./modules/udp2log/files/check_udp2log_procs:
./modules/varnish/files/check_vcl_reload:
./modules/varnish/files/confd-reload-vcl:
./modules/varnish/files/ganglia/check-gmond-restart:
./modules/varnish/files/reload-vcl:
./modules/varnish/files/varnish-backend-restart:
./modules/varnish/files/varnish-frontend-restart:
./modules/xenon/files/xenon-generate-svgs:
./utils/git-setup:
./utils/linter:
./utils/localrun:

That is a request similar to T144169 targeting python files

Slightly optimized based on what is in the git tree instead of the local checkout:

git ls-tree -z --name-only -r HEAD|xargs -0 file --mime-type|grep text/x-shellscript$|cut -d: -f1

Change 327592 had a related patch set uploaded (by Dzahn):
icinga/CI: give all shell scripts a file extension

https://gerrit.wikimedia.org/r/327592

gerritbot added a project: Patch-For-Review.Dec 15 2016, 9:54 PM

Change 327595 had a related patch set uploaded (by Dzahn):
installserver/CI: give shell scripts a file extension

https://gerrit.wikimedia.org/r/327595

Change 327673 had a related patch set uploaded (by Dzahn):
toollabs/CI: give banner scripts an .sh extension

https://gerrit.wikimedia.org/r/327673

I have a few scripts that are generated from templates. Any thoughts about what we can do for these cases?

@ArielGlenn it's surely depends on the specific cases, but I think that this is usually an anti-pattern. What you probably want is to have all that you need dynamically, moved to configuration files that are generated from templates so that the code can be properly validated and tested.

If this looks like an overkill in your cases because they are small single scripts, it's usually still possible to create a syntactically valid script for which some values will be replaced dynamically, like:

script_variable="<%= @puppet_variable %>"

It is still dangerous because if the puppet_variable contains double quotes will break the code. If we want to cover those cases too then we should try to validate also all the script that ends with .$ext.erb with $ext in a list of known languages extensions.

From a quick audit i can see that we have:

9 .py.erb
31 .sh.erb
7 .php.erb
0 .pl.erb

My 2 cents.

About spellcheck, Jessie has version 0.3.4 , would it make sense to backport 0.4.4 from testing and add it to our apt.wikimedia.org under jessie-wikimedia/third-parties ?

Paladox subscribed.Dec 16 2016, 11:03 AM

In T148494#2880204, @Volans wrote:

@ArielGlenn it's surely depends on the specific cases, but I think that this is usually an anti-pattern. What you probably want is to have all that you need dynamically, moved to configuration files that are generated from templates so that the code can be properly validated and tested.

FWIW I agree, interpolating erb in code (as opposed to configuration) usually makes things like decoupling/testing/validation harder

In T148494#2880204, @Volans wrote:
@ArielGlenn it's surely depends on the specific cases, but I think that this is usually an anti-pattern. What you probably want is to have all that you need dynamically, moved to configuration files that are generated from templates so that the code can be properly validated and tested.

If this looks like an overkill in your cases because they are small single scripts, it's usually still possible to create a syntactically valid script for which some values will be replaced dynamically, like:
script_variable="<%= @puppet_variable %>"

Now that you've said it, it seems obvious (like all great ideas). I won't get to this in the next few days but "soon". :-)

@ArielGlenn remember that if the file will still be a .py.erb it will be skipped by our tox checker as of now.
We could consider to add those files too to the ones that are checked (we have 9 right now in the puppet repo) as long as they pass the validation.

Change 327595 merged by Dzahn:
installserver/CI: give shell scripts a file extension

https://gerrit.wikimedia.org/r/327595

Change 327673 merged by Dzahn:
toollabs/CI: give banner scripts an .sh extension

https://gerrit.wikimedia.org/r/327673

Change 327592 merged by Dzahn:
icinga/CI: give all shell scripts a file extension

https://gerrit.wikimedia.org/r/327592

herron subscribed.Sep 8 2017, 4:34 PM

JeanFred mentioned this in T175906: Add Shell linting to heritage repo.Sep 14 2017, 11:18 AM

• Phabricator_maintenance moved this task from Backlog to Acknowledged on the SRE board.Jan 26 2019, 8:52 PM

A few years later, this an effort to run shell scripts validation via T254480 including scripts generated through .erb templates.

hashar mentioned this in T245266: Add a shell script linter.Jun 5 2020, 3:35 PM

Reopening since this is about adding shellcheck on any repository while T254480 is specific to puppet.git and covers liniting any .erb template.

Samwalton9-WMF subscribed.Jun 8 2020, 9:23 AM

hashar mentioned this in T259586: Add CI to mediawiki/tools/train-dev.Aug 4 2020, 7:25 AM

Dzahn mentioned this in T200984: Stop introducing new code expanded from erb templates.Oct 3 2020, 12:11 AM

I think this has been added via https://gerrit.wikimedia.org/r/c/operations/puppet/+/602693 can anyone confirm if we are still missing anything?

In T148494#6517546, @jbond wrote:

I think this has been added via https://gerrit.wikimedia.org/r/c/operations/puppet/+/602693 can anyone confirm if we are still missing anything?

Kind of. ShellCheck has been added as part of the work to normalize file extensions in operations/puppet.git in order to make it easier for linters to act on them. Which is T254480 and covers python files and various inlined shell snippets.

This task T148494 is to add generic support for ShellCheck beyond just operations/puppet. Which has two caveats:

how to find files to pass to the linter
how one can define the shellcheck version to use (or we can't enforce a specific version globally, that has proven unmanageable)

This task T148494 is to add generic support for ShellCheck beyond just operations/puppet. Which has two caveats:

how to find files to pass to the linter

For the puppet repo when discussing the python files it was agreed that all python files should have py extension and we have a check in CI to -1 if thats not the case, we could do something simlar for text/x-shellscript files?