fix(workflow-hook): log output with or without errors

[X-Guardian]

what

• In the pre and post workflow hook runners, the OutputHandler.SendWorkflowHook calls have been moved to before the command error handling.

why

• So that the pre and post workflow hook output always gets logged, irrelevant of whether an error occurred.

tests

repos.yaml:

repos:
- id: /.*/
  pre_workflow_hooks:
    - run: echo "pre workflow test line 1" && exit 0
    - run: echo "pre workflow test line 2" && exit 1
  post_workflow_hooks:
    - run: echo "post workflow test line 1" && exit 0
    - run: echo "post workflow test line 2" && exit 1

Output Screenshots

Pre Workflow Hook

Post Workflow Hook

Logs

Logs

{"level":"info","ts":"2023-05-20T12:56:49.413+0100","caller":"runtime/pre_workflow_hook_runner.go:77","msg":"successfully ran \"echo \\\"pre workflow test line 1\\\" && exit 0\" in \"atlantis-test\\\\1\\\\default\"","json":{"repo":"X-Guardian/atlantis-test","pull":"1"}}
{"level":"error","ts":"2023-05-20T12:56:50.396+0100","caller":"events/command_runner.go:293","msg":"Error running pre-workflow hooks exit status 1: running \"echo \\\"pre workflow test line 2\\\" && exit 1\" in \"atlantis-test\\\\1\\\\default\": \npre workflow test line 2\n. Proceeding with plan command.","json":{"repo":"X-Guardian/atlantis-test","pull":"1"},"stacktrace":"github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand\n\tatlantis/server/events/command_runner.go:293"}
{"level":"info","ts":"2023-05-20T12:56:51.405+0100","caller":"events/project_command_builder.go:399","msg":"found no atlantis.yaml file","json":{"repo":"X-Guardian/atlantis-test","pull":"1"}}
{"level":"info","ts":"2023-05-20T12:56:51.406+0100","caller":"events/project_command_builder.go:403","msg":"automatically determined that there were 0 projects modified in this pull request: []","json":{"repo":"X-Guardian/atlantis-test","pull":"1"}}
{"level":"info","ts":"2023-05-20T12:56:52.931+0100","caller":"runtime/post_workflow_hook_runner.go:77","msg":"successfully ran \"echo \\\"post workflow test line 1\\\" && exit 0\" in \"atlantis-test\\\\1\\\\default\"","json":{"repo":"X-Guardian/atlantis-test","pull":"1"}}
{"level":"error","ts":"2023-05-20T12:56:53.910+0100","caller":"events/command_runner.go:303","msg":"Error running post-workflow hooks exit status 1: running \"echo \\\"post workflow test line 2\\\" && exit 1\" in \"atlantis-test\\\\1\\\\default\": \npost workflow test line 2\n.","json":{"repo":"X-Guardian/atlantis-test","pull":"1"},"stacktrace":"github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunCommentCommand\n\tatlantis/server/events/command_runner.go:303"}

references

• Fixes Errors in Pre or Post Workflow Hook Scripts do not Produce any Output #3421

[nitrocode]

Brilliant! Thank you @X-Guardian for the deep dive and creating a fix. Is there a way we can add a unit test or e2e test or both to ensure this functionality doesn't break in the future?

[X-Guardian]

Hi @nitrocode, I did look at the current unit tests for the workflow hook runners, but couldn't obviously see how to change them to check for this scenario. From what I could see, they are currently checking the out return value of the Run function, which isn't actually used.

[nitrocode]

Is it not possible to modify or add to them to verify that your changes will also be tested? It must be possible.

This feature was working before and a follow up pr broke the feature and now this pr has been created to remediate the issue. Besides a thorough future review, what is preventing someone from creating a new pr and breaking the feature again? Only thing would be unit tests. Please consider adding them or we are doomed to repeat our mistakes.

[X-Guardian]

I didn't say it wasn't possible. I said that I couldn't see how to change them, as they are testing the output of the function not the internal workings of the function which is where the bug I fixed is.

This is my first PR on this repo, but looking at the history of the workflow hook status feature, this was introduced in PR #2441 and has always had this bug as you can see that the OutputHandler.SendWorkflowHook function call has always been after the error checks, that if triggered, will return from the function early.

[X-Guardian]

@nitrocode, this PR is ready for review.

[nitrocode]

Thank you! Pinging a few other reviewers too

cc @Fabianoshz @GenPage @jamengual @chenrui333