Lint rule for ensuring set -euo pipefail or similar is set

[a-frantz]

Upgrading discussion #60 to an issue for visibility.

Repeating myself, but I would think there's an "off the shelf" solution we can pull for this. It might involve shelling out, but I think that would be ok. I just don't want us to get in the business of writing a Bash parser 😬

[a-frantz]

I would rank this as a lower priority than #191 and implementing shellcheck might provide a simple solution to this. So this should probably take a backseat until we get shellcheck incorporated.

[dead8309]

Shellcheck is now(long ago) merged, I can work on it @a-frantz

[a-frantz]

@dead8309 I'm concerned this lint rule might not be achievable trivially, but I haven't investigated. I'm going to lay out some more thoughts here in this comment.

from the linked comment:

Many WDL tasks are single (pipeless) commands. We should not warn about a missing set -euo pipefail in those cases. That means we can't just blindly check that the string is present in the beginning of a command block, we will have to do some actual Bash parsing. That strikes me as something we want to avoid. I don't want to parse Bash.

To be more specific, we want this lint to fire if the command script contains more than one Bash command, either as individual invocations or as part of a stream (commands connected by pipes (|)). We don't want the lint to fire for single commands.

I think trying to parse the above naively (i.e. checking the command text for presence/absence of pipes, multiple lines, etc) will be too fragile. I'd rather not have this lint than have one that has false negatives or false positives.

That pretty much means we'll have to shell out to some existing tool for doing this check, and I'm not sure if that tool exists (I haven't looked, but I don't think this is included in shellcheck?).

So, I'm going to assign this issue to you, but coming back after some research and saying "this doesn't look doable" instead of pushing a PR is an acceptable outcome!

[dead8309]

@a-frantz we could use tree-sitter for parsing but yea you are right it doesn't look very doable, why would prople have tree-sitter grammars installed locally. And embedding it in the binary doesn't look right.

It might be better if we just document it as a best practice

[a-frantz]

@dead8309 yea I kind of expected this to lead to a dead end... that's too bad. I'm going to leave this issue open, because maybe someone finds a solution that works later!

It might be better if we just document it as a best practice

we already do this on the workflows repo! Although I'm not sure the whole doc is super up to date. I haven't reviewed it recently. https://github.com/stjudecloud/workflows/blob/main/best-practices.md