The job should fail when tofu fails by default

[giner]

Here is an example which should fail the job however it doesn't

$ tofu fmt --recursive --diff --check; echo $?
╷
│ Error: Invalid expression
│ 
│   on repos/repos.tf line 26, in locals:
│   26:     .myrepo = {
│ 
│ Expected the start of an expression, but found an invalid expression token.
╵

2

presumably because of this

setup-opentofu/wrapper/tofu.js

Line 47 in 26ccb9b

if (exitCode === 0 || exitCode === 2) {

One could argue that the default behavior should be for the job to fail if the executable fails. However, that isn't the case with the wrapper enabled by default. This leads to errors going unnoticed for a long time. I believe ignoring the exit code should be a conscious choice made by the user.

[diofeher]

Hi @giner, thanks for this enhancement request; it makes sense.

This repository is community-maintained, so we're happy to review if someone from the community implements it.

[giner]

I'm thinking what would make sense here. Exit codes for plan and fmt differ. How about the following?

• Remove the exception for exit code 2
• Add environment variable TOFU_WRAPPER_IGNORE_EXIT_CODES which takes a comma-separated list of codes
• Use TOFU_WRAPPER_IGNORE_EXIT_CODES in the wrapper tofu.js
• Mention TOFU_WRAPPER_IGNORE_EXIT_CODES in the docs

[giner]

@diofeher how does the above sound to you?

[apparentlymart]

It's unfortunate that we've ended up with exit status 2 having at least three different incompatible meanings across different situations:

• tofu plan's -detailed-exitcode option causes it to use exit status 2 to represent "no changes are needed".

  This particular use is explicitly documented and so definitely covered by our compatibility promises.

• The Go runtime uses exit status 2 whenever it reports a fatal panic.

  We have some code in OpenTofu that makes a best effort to use exit status 11 instead in that case specifically to avoid it colliding with the -detailed-exitcode meaning, but due to the nature of panics in Go that isn't 100% reliable.

• tofu fmt appears to use exit status 2 for errors related to loading the configuration files that are to be formatted, differentiating those from other kinds of errors.

  However, the specific exit status categories used by this command don't seem to be documented anywhere and it seems like whatever design motivation was originally present here has been forgotten long ago. We could probably change this to use a different status code without upsetting anything significantly, because the set of problems that cause exit status 2 doesn't seem like cases that automation would need to treat differently than any other exit code.

All of that said: I feel tempted to suggest that we change tofu fmt to not use exit status 2 and then decide that exit status 2 is reserved to only have the special -detailed-exitcode meaning and would never be used in any other case. That would then make the current logic in setup-opentofu more reasonable; the code says that -detailed-exitcode was what this special case was intended to handle.

That would still leave the risk of panics we can't intercept being treated as successes though, which remains highly unfortunate. That choice is hard-coded into the Go runtime and so we don't really have any options beyond what we're already doing.

The wrapper program has full access to the array of argument strings though, so maybe we could promise ourselves that we'll never use the option -detailed-exitcode to mean anything other than what it currently means for tofu plan, and have the wrapper script search args to see if -detailed-exitcode or --detailed-exitcode appears anywhere in it, and then make the exception for exit status 2 only when it's present. That heuristic would then make it continue to handle -detailed-exitcode's unusual behavior the same way it used to, but only when that argument is actually being used. It's unclean in the sense of being a layering violation, but it seems unlikely that we'd accidentally break it in future as long as -detailed-exitcode remains bound by the v1.x compatibility promises.