-
Notifications
You must be signed in to change notification settings - Fork 308
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
SCT now handles exceptions thrown by execution listeners #344
Conversation
this.applicationFailedEvent.getException()).getExitMessage()); | ||
setMessages(invokeOnTaskEnd(this.taskExecution)); | ||
if(this.listenerFailed) { | ||
this.taskExecution.setExitCode(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be hardcoded as 1, a specific value that we specify other than one (to indicate the specific error condition being an exception was thrown via a SCT listener), or should this be the result of any configured ExitCodeGenerator
? Where in the lifecycle does the ExitCodeGenerator
get called based on this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR we stop processing all TaskExecutionListeners when an exception is thrown by a listener. So if I understand you correctly, you want to check to see if the exception thrown by the listener implements the ExitCodeGenerator interface and if so use that exit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I wouldn't expect an exception to implement ExitCodeGenerator
. However, there may be one in the ApplicationContext
. I'm asking if we should use it...
this.listenerFailed = true; | ||
this.taskExecution.setErrorMessage(listenerException.getMessage()); | ||
this.listenerException = listenerException; | ||
throw listenerException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad practice to use duplicate variable names. Please use something different.
This PR should also have some form of update of documentation associated with it. Both reference docs and java docs need to be updated to document the lifecycle change. |
Documentation updated as well. |
Rebased, squashed, and merged as 3f316a0 |
resolves #341