- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: propagate job error codes#6507
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
BrunoQuaresma left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
That is nice! Would be great to have the errors being exported on typesGenerated as:
exportconstErrors={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
mafredri left a comment
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.
A few comments but looks good otherwise 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1 @@ | |||
| ALTERTABLE provisioner_jobs ADD COLUMN error_codetext DEFAULTNULL; | |||
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.
Could we potentially run into multiple errors? Would it make sense to store an array here?
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.
I would stick to theerror_code at the moment. Most (all?) flows fail fast (return failJob(...)), so there is no simple way to aggregate multiple errors.
Moreover, we have singleerror property. In this case, we would neederrors [] as well.
| code=MissingParameterErrorCode | ||
| }elseifstrings.Contains(message,requiredTemplateVariablesErrorText) { | ||
| code=RequiredTemplateVariablesErrorCode | ||
| } |
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.
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?
varerrorCodes=map[string]string{MissingParameterErrorCode:missingParameterErrorText,// ...}forc,m:=rangeerrorCodes {// ...}
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 anerr error which could be a predefined&jobError{m: "missing x", c: "MISSING_X}.
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.
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...
Uh oh!
There was an error while loading.Please reload this page.
mtojek commentedMar 8, 2023
@BrunoQuaresma I added these errors to |
| var_error=new(ProvisionerJobError) | ||
| func (err*ProvisionerJobError)Error()string { | ||
| returnerr.Message |
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.
Just an idea, could be useful sometime.
| returnerr.Message | |
| returnfmt.Sprintf("%s (%s)",err.Message,err.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.
Thanks for suggestion, but this message will be shown in CLI or site, let's not scare the customer :)
| CompletedAt*time.Time`json:"completed_at,omitempty" format:"date-time"` | ||
| CanceledAt*time.Time`json:"canceled_at,omitempty" format:"date-time"` | ||
| Errorstring`json:"error,omitempty"` | ||
| ErrorCodeJobErrorCode`json:"error_code,omitempty" enums:"MISSING_TEMPLATE_PARAMETER,REQUIRED_TEMPLATE_VARIABLES"` |
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.
Question: Is theenums: still required for code gen even after the update ofswaggo? In my experience it has been able to auto-detect but it might be very specific.
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.
I checked it now - unfortunately, swaggo removed enum parts from the docs andswagger.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.