feat: propagate job error codes #6507
Conversation
There was a problem hiding this comment.
That is nice! Would be great to have the errors being exported on typesGenerated as:
export const Errors = {
REQUIRED_TEMPLATE_VARIABLES: "REQUIRED_TEMPLATE_VARIABLES",
...
}So we could easily find out what are the available error codes and use as:
someError === Errors.REQUIRED_TEMPLATE_VARIABLES
| @@ -0,0 +1 @@ | |||
| ALTER TABLE provisioner_jobs ADD COLUMN error_code text DEFAULT NULL; | |||
There was a problem hiding this comment.
Could we potentially run into multiple errors? Would it make sense to store an array here?
There was a problem hiding this comment.
I would stick to the error_code at the moment. Most (all?) flows fail fast (return failJob(...)), so there is no simple way to aggregate multiple errors.
Moreover, we have single error property. In this case, we would need errors [] as well.
| code = MissingParameterErrorCode | ||
| } else if strings.Contains(message, requiredTemplateVariablesErrorText) { | ||
| code = RequiredTemplateVariablesErrorCode | ||
| } |
There was a problem hiding this comment.
This format seems like it'd be easy to miss, e.g. when adding new error messages/codes.
How about storing them in a map and iterating over it here?
var errorCodes = map[string]string{
MissingParameterErrorCode: missingParameterErrorText,
// ...
}
for c, m := range errorCodes {
// ...
}Ultimately though, this logic is a bit hidden behind a "print function", it might be a good idea to refactor so that it can accept an err error which could be a predefined &jobError{m: "missing x", c: "MISSING_X}.
There was a problem hiding this comment.
Good idea with the map, added.
Ultimately though, this logic is a bit hidden behind a "print function", it might be a good idea to refactor so that it can accept an err error which could be a predefined &jobError{m: "missing x", c: "MISSING_X}.
This is risky, as it would require refactoring of interfaces, and I'm sure that I don't want to deal with it now...
|
@BrunoQuaresma I added these errors to |
| var _ error = new(ProvisionerJobError) | ||
|
|
||
| func (err *ProvisionerJobError) Error() string { | ||
| return err.Message |
There was a problem hiding this comment.
Just an idea, could be useful sometime.
| return err.Message | |
| return fmt.Sprintf("%s (%s)", err.Message, err.Code) |
There was a problem hiding this comment.
Thanks for suggestion, but this message will be shown in CLI or site, let's not scare the customer :)
| @@ -75,6 +83,7 @@ type ProvisionerJob struct { | |||
| CompletedAt *time.Time `json:"completed_at,omitempty" format:"date-time"` | |||
| CanceledAt *time.Time `json:"canceled_at,omitempty" format:"date-time"` | |||
| Error string `json:"error,omitempty"` | |||
| ErrorCode JobErrorCode `json:"error_code,omitempty" enums:"MISSING_TEMPLATE_PARAMETER,REQUIRED_TEMPLATE_VARIABLES"` | |||
There was a problem hiding this comment.
Question: Is the enums: still required for code gen even after the update of swaggo? In my experience it has been able to auto-detect but it might be very specific.
There was a problem hiding this comment.
I checked it now - unfortunately, swaggo removed enum parts from the docs and swagger.json, so I will leave it as is.
Related: #6100
This PR extends a provisioner job runner to return error codes related to specificc failures. So far we need two different codes:
MISSING_TEMPLATE_PARAMETERandREQUIRED_TEMPLATE_VARIABLES.