Shellcheck and shfmt on bash scripts

[KaushikMalapati]

Description

I accidentally changed the branch for the old pr #184 which automatically closed it, so I am making a new one. I added a pre-commit job for shfmt which will automatically format bash scripts and went through all existing shellcheck errors/warnings that the existing job raises. I am disabling some warnings globally in .shellcheckrc and on a line-by-line basis in a few files.

Motivation and Context

https://github.com/pcdshub/engineering_tools/issues/182 To standardize style and fix existing shellcheck issues so that future contributors only see pre-commit/workflow errors related to their changes instead of what was already there.

How Has This Been Tested?

Interactively

Where Has This Been Documented?

N/A

[tangkong]

If I understand correctly, this is a re-creation of the previous shellcheck PR we spent some time reviewing before right? Or were there other changes made in addition? If I recall correctly, our main issue was that this touched a lot of files and we don't have a great way of regression testing the changes made here (short of manually testing the scripts).

I still think this looks largely un-problematic, though I might feel better if we approached this in pieces. Or maybe we just rip off the bandaid and make sure we're ready to hotfix any unexpected breaks

[ZLLentz]

The main differences I see between this and previous are the addition of the shfmt pre-commit and applying fixes to files added since the last PR. I'm going to continue reviewing.

[ZLLentz]

I think this is good if anyone is brave enough to merge it

[tangkong]

I'm scared tbh. After we merge, should we tag/update asap to get people using these scripts? This will be the root of many a merge conflict I'm sure

[silkenelson]

I have a pull request to add needed functionality to makepeds which failed pre-commit which will have to resolved and I don't even have the time to fix the current problems. Would there be a chance to not push changes to 56 files at the same time, in particular during operations? We need to make operational needed changes and I'm also concerned this will be messy.

[ZLLentz]

Silke, this repo is not gating merges on pre-commit failures, they are currently optional, so for your PR you should proceed as normal given the time investment possible. The only merge requirement here is "one passing review".

For this PR I suspect we want to at least test the scripts that have gotten more than surface-level (whitespace) edits. Maybe the next step for this one is to collect the scripts that need to be re-tested.

[KaushikMalapati]

I think these scripts should be retested

• camViewer
• ioctool
• ipmConfigEpics
• makepeds
• makepeds_psana
• motorInfo
• pcds_conda
• restartdaq
• serverStat
• startami
• wherepsana

[ZLLentz]

I skimmed the PR again and I think I agree with those choices, some may not necessarily need it but it's better to be thorough. The only questions are then:

• Who should test these? Maybe we can split up the work a bit?
• How should each be tested?
• Do we need to split up the PR to get in the simple ones first, or is that not worth the effort?

[tangkong]

I'm in favor of splitting these up, which to me doesn't sound like too much git-fu but I realize it could be annoying.

I can't say that I'd be very useful in testing these scripts, but I do dream of a day when the test procedure is a little more robust than "run the script and see if it breaks". I assume there's been no real testing procedure in place? Is there even a way to test these without building mock services for all the things these scripts touch?