T feyiadeaga error clarification#55
Conversation
|
I decided to get log errors in this PR by using the existing exitcodes and converting them to corresponding errors that are added to the status report object that is reported. |
| case ExitCode_Okay: | ||
| return 0 // Success, no error clarification needed | ||
|
|
||
| // User errors (-100s) -> map to positive user error codes |
There was a problem hiding this comment.
You don't need this mapping for user errors. Directly change ExitCode_* to new positive number for user errors as expected by CRP. Linux returns positive error codes. Please also check what other Linux extensions are doing like CustomScript
There was a problem hiding this comment.
Agreed. In RunCommand V2 Windows we just return the error clarification as an int for the exit code.
There was a problem hiding this comment.
Or in the case of the implementation here it would be the opposite.
| case ExitCode_RunAsLookupUserFailed: | ||
| return CommandExecution_RunAsUserLogonFailed | ||
|
|
||
| // Service errors (-200s) -> map based on specific failure type |
There was a problem hiding this comment.
same here. no mapping needed. You can directly use the negative code for service errors. Does CRP expect negative service code for service error ?
|
|
||
| // Overwrite function to report status to HGAP. This function prepares the status to be sent to the HGAP and then calls the notifier to send it. | ||
| cmd.Functions.ReportStatus = func(ctx *log.Context, _ types.HandlerEnvironment, metadata types.RCMetadata, statusType types.StatusType, c types.Cmd, msg string) error { | ||
| cmd.Functions.ReportStatus = func(ctx *log.Context, _ types.HandlerEnvironment, metadata types.RCMetadata, statusType types.StatusType, c types.Cmd, msg string, exitcode ...int) error { |
There was a problem hiding this comment.
It is optional. In the use case only enable functions pass the value
| // | ||
| // This function is used by default for reporting status to the local file system unless a different method is specified. | ||
| func ReportStatusToLocalFile(ctx *log.Context, hEnv types.HandlerEnvironment, metadata types.RCMetadata, statusType types.StatusType, c types.Cmd, msg string) error { | ||
| func ReportStatusToLocalFile(ctx *log.Context, hEnv types.HandlerEnvironment, metadata types.RCMetadata, statusType types.StatusType, c types.Cmd, msg string, exitcode ...int) error { |
There was a problem hiding this comment.
Is the exitcode ...int a hack around go not allowing polymorphism of functions?
There was a problem hiding this comment.
I used it to pass an optional parameter to the already existing function
| case ExitCode_Okay: | ||
| return 0 // Success, no error clarification needed | ||
|
|
||
| // User errors (-100s) -> map to positive user error codes |
There was a problem hiding this comment.
Agreed. In RunCommand V2 Windows we just return the error clarification as an int for the exit code.
| "testing" | ||
| ) | ||
|
|
||
| func TestTranslateExitCodeToErrorClarification(t *testing.T) { |
There was a problem hiding this comment.
If we just convert the error clarification to an int, then we don't need this test.
We should also try to repro each condition in a test case, since this improves the overall quality of the extension.
| goalStateKey := key.(types.GoalStateKey) | ||
| if goalStateKey.RuntimeSettingsState != "disabled" { | ||
| if value.(types.StatusItem) != (types.StatusItem{}) { | ||
| if !IsEmptyStatusItem(value.(types.StatusItem)) { |
There was a problem hiding this comment.
Not following why this is necessary. Please at least add a comment stating why we need the DeepEqual call.
| case ExitCode_Okay: | ||
| return 0 // Success, no error clarification needed | ||
|
|
||
| // User errors (-100s) -> map to positive user error codes |
There was a problem hiding this comment.
Or in the case of the implementation here it would be the opposite.
| return errors.Wrap(err, "failed to get json for status report") | ||
| } | ||
|
|
||
| ctx.Log("message", "reporting status by writing status file locally") |
There was a problem hiding this comment.
Please check whether we already have a trace stating which status we're writing. If not, then this trace should provide the status and error clarification too.
| } | ||
| func getRootStatusJsonWithErrorClarification(ctx *log.Context, statusType types.StatusType, c types.Cmd, msg string, indent bool, extName string, errorcode int) ([]byte, error) { | ||
| ctx.Log("message", "creating json to report status") | ||
| statusReport := types.NewStatusReportWithErrorClarification(statusType, c.Name, msg, extName, errorcode) |
There was a problem hiding this comment.
I believe elsewhere in the code we also write status, so this would overwrite it.
5 participants
Adding ErrorClarification for RunCommandV2 on linux