feat: Commands should fail the build if their exit code is not zero

[Fran-Rg]

Description

When running with some source_path setting a command:

source_path = [
        {
          "commands" = XYZ
        }

It might happen the commands fails and we want to fail the terraform deployment

Motivation and Context

Before packaging we needed to set some auth token for npm resources. In some rare cases the authentication failed but the build carried on.

Breaking Changes

No

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects

• I have executed pre-commit run -a on my pull request

[antonbabenko]

I am just double-checking with you, will we also see the text of the error?

@pdecat WDYT?

[pdecat]

Without any testing, the changes look good to me.

However, I'm a bit confused about the closing of the w file descriptor right above.

[pdecat]

I'd trace the exit code for easier debugging:

Suggested change

	"Script did not run successfully"
	"Script did not run successfully, exit code {}".format(exit_code)

[pdecat]

Shouldn't this be closed after p.wait() has returned?

[Fran-Rg]

I don't understand why the test is failing on Github Action. Works fine locally.

[antonbabenko]

@Fran-Rg I think you need to fix the conflicts, and then GitHub can restart it automatically. If it doesn't happen by itself, I can hit the restart button manually.

[Fran-Rg]

I ended up reformatting the code as the fd wasn't being used. Outputs get shown on failure

[pdecat]

I ended up reformatting the code as the fd wasn't being used.

The r and w file descriptors from the pipe were used, but I believe w was closed too early which may have caused issues if the command was too slow to complete.

If we look at this code fragment from v6.8.0:

            elif cmd == 'sh':
                r, w = os.pipe()
                side_ch = os.fdopen(r)
                path, script = action[1:]
                script = "{}\npwd >&{}".format(script, w)

                p = subprocess.Popen(script, shell=True, cwd=path,
                                     pass_fds=(w,))
                os.close(w)
                sh_work_dir = side_ch.read().strip()
                p.wait()
                log.info('WD: %s', sh_work_dir)
                side_ch.close()

w is passed to the sub-process and written to with the pwd >&{} redirection.

The result of pwd is then read from the other end of the pipe with side_ch.read().

[Fran-Rg]

But then the result of pwd is never used unless I'm missing something

[pdecat]

But then the result of pwd is never used unless I'm missing something

It was just logged in log.info('WD: %s', sh_work_dir)

[Fran-Rg]

I believe this is makes it not powershell friendly since set -e is only linux friendly, $ErrorActionPreference="Stop" needs to be used instead for powershell

[pdecat]

In my opinion, passing set -e or anything else should be left to the user of the terraform module.

[Fran-Rg]

That is right, I've added a comment. So initially what was causing the exit code not caring about the actual problem was the "pwd" being appended. Since it's for a log only removing it cleared the problem

[pdecat]

Awesome, thanks for adding the unit tests!

I'll try this PR ASAP.

[pdecat]

Maybe rebase/squash some git commits to cleanup history.

[antonbabenko]

This PR is included in version 7.1.0 🎉

[antonbabenko]

There was no need to prepare git history because we always squash commits during PR merge.

Thank you!

[sohaibiftikhar]

Unfortunately after this change debugging of custom commands becomes a lot harder since the logs from stdout/stderr are printed only on failure. In addition before we had streaming logs due to the pipes. Now the stderr and out are separated out which makes it harder to correlate them.

Also I believe the wrong logger is used? I had to change log.info to self._log.info to see exit code results.

[p0fi]

Just for completeness: I believe this PR finally fixes #388

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.