-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
-detailed-exitcode
causes a wrong exit code to be emitted when there was a retry
#3845
Comments
I suspect the error to hide here: https://sourcegraph.com/github.com/gruntwork-io/terragrunt@f9eb618e4a8a41facbee47d3c9b6042752596f0f/-/blob/shell/run_shell_cmd.go?L75-78 as the exit code processing is only done when there was an actual error and the previously set error code probably remains in the context and used once the function exits. |
Thanks for reporting this, @juljaeg ! It seems like you're also familiar with the codebase, and have found the source of the bug. Are you interested in contributing a pull request to patch the bug? |
I can try, but when I look in the past where I tried to work on the terragrunt code base this was not very successful 😆 I imagine it's mainly rearranging the condition block to look something like this: output, err := RunShellCommandWithOutput(ctx, opts, "", false, needsPTY, opts.TerraformPath, args...)
code, _ := util.GetExitCode(err)
if exitCode := DetailedExitCodeFromContext(ctx); err != nil && exitCode != nil {
exitCode.Set(code)
}
if util.ListContainsElement(args, terraform.FlagNameDetailedExitCode) && code != 1 {
return output, nil
}
return output, err Does that sound right? But I am definitely not familiar with possible side effects of this, let alone writing a proper unit/integration test for this 😕 I am not sure whether this is the right place to do the evaluation, shouldn't it be somewhere around here: terragrunt/cli/commands/terraform/action.go Line 340 in f9eb618
There one can be more certain that all retries have been done and the final result is there. So we don't need to take care of "reverting" the changes from retried run. I imagine in a concurrency scenario this might be unreliable. EDIT 6: Also thank you for the (very) fast response 😉 |
If you're not comfortable with cutting the pull request, don't worry! We'll mark the pull request as preserved so that it doesn't go stale, and we'll get to it when we have bandwidth. If you are interested in ramping up for contributing to Terragrunt, join the Terragrunt Discord if you haven't already, and read the Contribution docs. Especially when it comes to edge cases like this, it's extremely valuable to the maintainers to have community members contributing to help us cover all our edge cases. You've already written a viable fixture, which could go here (though I'd recommend that you try the retry block instead of the deprecated attributes like |
Describe the bug
When using
-detailed-exitcode
this causes a wrong exit code to be emitted when there was a retry according to the terragrunt config. It should show the exit code 0 if the retry was successful.Steps To Reproduce
Configure some testing suitable retry values:
Put a dummy resource which executes the script and prints the script name on failure (i.e. triggering a retry):
The script will fail on the first run but succeed on the second:
Expected behavior
Works as it should and gives us exit code 0.
Will show exit code 1, although the retry was successful. It should be exit code 0. Also visible in the terminal output:
Versions
PS
Thank you for your great work! 😄
The text was updated successfully, but these errors were encountered: