Improve handling startup error on Piped #2732
Conversation
| if err := execute(); err != nil { | ||
| input.Logger.Error("LAUNCHER: failed while checking whether relaunch required or relaunching. Skip the loop this time", zap.Error(err)) | ||
| // Don't return an error to continue piped execution. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
If you want to print out the error, making it print inside the execute() function is better since we have the cause in detail and can print the error log more clearly.
There was a problem hiding this comment.
There already exists logging error detail inside execute(), Should I delete this logging?
There was a problem hiding this comment.
And this return (with or without err) will stop the execution of the launcher and we don't want that, so I think it's better to keep the execution in this loop as is.
There was a problem hiding this comment.
this return will stop the execution of launcher
I made a mistake 😱 I'll fix it.
According to the issue, I want to confirm something about the context. The docs around Also, want other people's opinions. |
|
@kurochan Nice pull request. Thank you. 😍 |
I think he already updated the PR to be like that. |
|
/trigger presubmits |
|
@nghialv: Your requested presubmits has been scheduled in response to this comment. |
|
/lgtm |
|
Code coverage for golang is
|
Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com>
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
/golinter trigger |
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
/trigger presubmits |
|
@khanhtc1202: Your requested presubmits has been scheduled in response to this comment. |
|
Looks like @pipecd-bot takes his rest today 🏖️ |
|
Code coverage for golang is
|
|
Nice improvement 👍 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Handle execution loop error.
Which issue(s) this PR fixes:
Fixes #2731
Does this PR introduce a user-facing change?: