Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure workflow status failure if command fails in backendless mode #1760

Merged

Conversation

minamijoyo
Copy link
Contributor

Fixes #1658

Error handling in backendless mode was incorrect, resulting in the workflow exit status to be success even if the command execution failed. This patch addresses this issue.

The following are some additional notes of this fix:

  • When allAppliesSuccessful is false, we need to call ReportErrorAndExit() to ensure that the workflow fails with a non-zero exit status.
  • Since ReportErrorAndExit() invokes os.Exit() immediately, it should be called after setting the status of the pull request.
  • Contrary to intuition, atLeastOneApply is counted even if plan command, so we need to use scheduler.IsPlanJobs() to determine if the current workflow is plan or apply.

Fixes diggerhq#1658

Error handling in backendless mode was incorrect, resulting in the
workflow exit status to be success even if the command execution failed.
This patch addresses this issue.

The following are some additional notes of this fix:

- When `allAppliesSuccessful` is false, we need to call
  `ReportErrorAndExit()` to ensure that the workflow fails with a
  non-zero exit status.
- Since `ReportErrorAndExit()` invokes `os.Exit()` immediately, it
  should be called after setting the status of the pull request.
- Contrary to intuition, `atLeastOneApply` is counted even if plan
  command, so we need to use `scheduler.IsPlanJobs()` to determine if
  the current workflow is plan or apply.
@ben-of-codecraft
Copy link
Contributor

I like this change. I am cherry picking for our fork.

@motatoes
Copy link
Contributor

motatoes commented Nov 4, 2024

Thanks @minamijoyo for this piece! Much appreciated

@motatoes motatoes merged commit 63ff5a2 into diggerhq:develop Nov 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Digger reports success even though Terraform apply fails
3 participants