ShellCheck?

[reidsunderland]

I'm using a GitHub Action that runs ShellCheck for sr3_tools and it's been working well to pre-detect issues in shell scripts.

It might be nice to run ShellCheck on sr_insects, implement it's suggested fixes and then set up the same Action to automatically run it.

Slightly related to MetPX/sarracenia#82.

[reidsunderland]

I applied the shellcheck fixes to static_flow. Interestingly, it took multiple runs of shellcheck to clean up all errors. I just kept running it until there was nothing left to fix.

There are also a quite a few things that it can't automatically fix. It reports: Issues were detected, but none were auto-fixable. Use another format to see them. Examples:

• SC2034 (warning): totsubcp appears unused. Verify use (or export if used externally).
• SC2126 (style): Consider using grep -c instead of grep|wc -l.

The auto changes caused one error for static_flow:

stat: cannot stat '/home/reid/.cache/sr3/log/watch_*.log': No such file or directory
test 1 FAILURE: Log perms test failed.

The change it made was:

-    fileperms=`stat -c "%a %n" $path`
+    fileperms=`stat -c "%a %n" "$path"`

In flow_check.sh line 114:
    fileperms=$(stat -c "%a %n" $path)
                                ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

The backtick to $() change was applied in a subsequent run.

To ignore particular errors/warnings, you can add # shellcheck disable=SC2059 above the line, so I did that for this line.

I pushed the the changes to the shellcheck branch.

It is useful for catching errors and keeping the style consistent, but it's kind of hard to say if the changes are actually helping here or not. I think we would have to get the numbers of things Shellcheck reports as an error down to 0 before it would be useful for finding new errors that people might add...

[petersilva]

I reviewed the commit in the branch. I think the changes are helpful, as the added quoting makes it harder to have parsing errors (end up with "" instead of nothing, when vars have no value.) And it will eliminate noise and find other errors presumably. I think it's good to go...

[reidsunderland]

I'll try it on the other tests! :)

[reidsunderland]

It worked on restart_server as well: 6c6ef48

[reidsunderland]

Flakey_broker worked 8639e5d as well. But applying the fixes to dynamic seems to have broken it. I'm trying to find the problem

[reidsunderland]

The dynamic_flow had these errors from flow_limit.sh but everything passed in flow_check.sh

grep: unlinked: No such file or directory
grep: [1-3]": No such file or directory
Initial v3 sample building sample size 0 need at least 700 
Waiting to start...
Waiting to start...
Done Waiting (sample now:3)
grep: unlinked: No such file or directory
grep: [1-3]": No such file or directory
Sample now:    164 Missed_dispositions:0
grep: unlinked: No such file or directory
grep: [1-3]": No such file or directory
Sample now:    226 Missed_dispositions:0
grep: unlinked: No such file or directory
grep: [1-3]": No such file or directory
Sample now:    342 Missed_dispositions:0
grep: unlinked: No such file or directory
grep: [1-3]": No such file or directory

The line that caused the problem was

# Original
countthem "`grep -aE \"\[INFO\].* unlinked [1-3]\" "$LOGDIR"/shovel_pclean_f92*.log | wc -l`"

# Shellcheck's fix
countthem "$(grep -aE \"\[INFO\].* unlinked [1-3]\" "$LOGDIR"/shovel_pclean_f92*.log | wc -l)"

# Manual fix
countthem "$(grep -aE '\[INFO\].* unlinked [1-3]' "$LOGDIR"/shovel_pclean_f92*.log | wc -l)"

I also found a problem with flow_setup.sh for all tests where the old logs weren't being deleted. Fixed with a4741ae

rm: cannot remove './watch_f40_01.log'$'\n''./subscribe_ftp_f70_03.log'$'\n''./shovel_pclean_f90_02.log'$'\n''./subscribe_cp_f61_01.log'$'\n''./subscribe_q_f71_01.log'$'\n''./cpost_veille_f34_01.log'$'\n''./subscribe_amqp_f30_01.log'$'\n''./poll_f62_01.log'$'\n''./subscribe_cp_f61_02.log'$'\n''./sarra_download_f20_03.log'$'\n''./subscribe_cclean_f91_02.log'$'\n''./shovel_t_dd2_f00_02.log'$'\n''./subscribe_u_sftp_f60_02.log'$'\n''./subscribe_cfile_f44_02.log'$'\n''./sarra_download_f20_02.log'$'\n''./sarra_download_f20_01.log'$'\n''./subscribe_cfile_f44_01.log'$'\n''./sender_tsource2send_f50_07.log'$'\n''./sender_tsource2send_f50_08.log'$'\n''./flowcleanup_f99.log'$'\n''./subscribe_cp_f61_03.log'$'\n''./srposter_f00.log'$'\n''./shovel_t_dd2_f00_03.log'$'\n''./subscribe_amqp_f30_02.log'$'\n''./subscribe_cdnld_f21_02.log'$'\n''./shovel_t_dd1_f00_02.log'$'\n''./sender_tsource2send_f50_05.log'$'\n''./winnow_t00_f10_01.log'$'\n''./trivialhttpserver_f00.log'$'\n''./subscribe_ftp_f70_01.log'$'\n''./subscribe_q_f71_03.log'$'\n''./shovel_rabbitmqtt_f22_03.log'$'\n''./winnow_t01_f10_01.log'$'\n''./cpump_xvan_f15_01.log'$'\n''./sender_tsource2send_f50_04.log'$'\n''./shovel_t_dd2_f00_01.log'$'\n''./subscribe_q_f71_02.log'$'\n''./shovel_pclean_f92_03.log'$'\n''./subscribe_rabbitmqtt_f31_03.log'$'\n''./flowcheck_ERROR_logged.txt'$'\n''./sender_tsource2send_f50_09.log'$'\n''./flow_setup.exchanges.txt'$'\n''./cpump_xvan_f14_01.log'$'\n''./subscribe_cclean_f91_03.log'$'\n''./shovel_pclean_f92_02.log'$'\n''./shovel_pclean_f90_01.log'$'\n''./shovel_pclean_f92_01.log'$'\n''./sender_tsource2send_f50_01.log'$'\n''./cpump_pelle_dd1_f04_01.log'$'\n''./subscribe_rabbitmqtt_f31_01.log'$'\n''./sender_tsource2send_f50_10.log'$'\n''./subscribe_ftp_f70_02.log'$'\n''./subscribe_cclean_f91_01.log'$'\n''./shovel_pclean_f90_03.log'$'\n''./shovel_t_dd1_f00_01.log'$'\n''./shovel_rabbitmqtt_f22_02.log'$'\n''./shovel_rabbitmqtt_f22_01.log'$'\n''./subscribe_cdnld_f21_01.log'$'\n''./shovel_t_dd1_f00_03.log'$'\n''./subscribe_amqp_f30_03.log'$'\n''./cpump_pelle_dd2_f05_01.log'$'\n''./trivialftpserver_f00.log'$'\n''./sender_tsource2send_f50_06.log'$'\n''./subscribe_u_sftp_f60_01.log'$'\n''./flowcheck_WARNING_logged.txt'$'\n''./subscribe_cfile_f44_03.log'$'\n''./subscribe_u_sftp_f60_03.log'$'\n''./sender_tsource2send_f50_02.log'$'\n''./subscribe_cdnld_f21_03.log'$'\n''./subscribe_rabbitmqtt_f31_02.log'$'\n''./sender_tsource2send_f50_03.log'$'\n''./sums/downloaded_by_sub_cp.txt'$'\n''./sums/downloaded_by_sub_amqp.txt'$'\n''./sums/sent_by_tsource2send.txt'$'\n''./sums/posted_by_shim.txt'$'\n''./sums/downloaded_by_sub_rabbitmqtt.txt'$'\n''./sums/cfile.txt'$'\n''./sums/recd_by_srpoll_test1.txt'$'\n''./sums/downloaded_by_sub_u.txt'$'\n''./sums/linked_by_shim.txt'$'\n''./sums/cfr.txt': No such file or directory

[petersilva]

I just looked at patches on sr_insects, noticed this one languishing... it was a good idea...

I'm not seeing any commits on this one... maybe we back up, and do separate commits for each test we get working, rather than a PR for the whole thing. in the year since this has happenned there are new tests copied from static_flow, and so the longer things go, the more places there are to make these changes.

[petersilva]

Might be good to capture some rules... preferences for the linter.