Skip to content

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Mar 8, 2023
Merged

feat: propagate job error codes #6507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b77747b
feat: propagate job error_code
mtojek Mar 8, 2023
bfa9140
fix
mtojek Mar 8, 2023
e41136e
Fix
mtojek Mar 8, 2023
55f46b7
Fix
mtojek Mar 8, 2023
77d7d7d
Fix
mtojek Mar 8, 2023
43e6b7a
Merge branch 'main' into 6100-error-codes
mtojek Mar 8, 2023
ee381ab
add errors to typesGenerated
mtojek Mar 8, 2023
20fe286
Address PR comments
mtojek Mar 8, 2023
d782120
Fix
mtojek Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address PR comments
  • Loading branch information
@mtojek
mtojek committed Mar 8, 2023
commit 20fe286c64dfa63a22501760d4151886350757d6
14 changes: 5 additions & 9 deletions cli/cliui/provisionerjob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,14 @@ type ProvisionerJobOptions struct {
}

type ProvisionerJobError struct {
message string
code codersdk.JobErrorCode
Message string
Code codersdk.JobErrorCode
}

var _ error = new(ProvisionerJobError)

func (err *ProvisionerJobError) Error() string {
return err.message
}

func (err *ProvisionerJobError) Code() codersdk.JobErrorCode {
return err.code
return err.Message
Copy link
Member

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.

Suggested change
return err.Message
return fmt.Sprintf("%s (%s)", err.Message, err.Code)

Copy link
Member Author

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 :)

}

// ProvisionerJob renders a provisioner job with interactive cancellation.
Expand Down Expand Up @@ -197,8 +193,8 @@ func ProvisionerJob(ctx context.Context, writer io.Writer, opts ProvisionerJobOp
case codersdk.ProvisionerJobFailed:
}
err = &ProvisionerJobError{
message: job.Error,
code: job.ErrorCode,
Message: job.Error,
Code: job.ErrorCode,
}
jobMutex.Unlock()
flushLogBuffer()
Expand Down
4 changes: 2 additions & 2 deletions cli/templatecreate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVers
})
if err != nil {
var jobErr *cliui.ProvisionerJobError
if errors.As(err, &jobErr) && !provisionerd.IsMissingParameterError(string(jobErr.Code())) {
if errors.As(err, &jobErr) && !provisionerd.IsMissingParameterErrorCode(string(jobErr.Code)) {
return nil, nil, err
}
}
Expand Down Expand Up @@ -235,7 +235,7 @@ func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVers
}
}

if provisionerd.IsMissingParameterError(string(version.Job.ErrorCode)) {
if provisionerd.IsMissingParameterErrorCode(string(version.Job.ErrorCode)) {
valuesBySchemaID := map[string]codersdk.ComputedParameter{}
for _, parameterValue := range parameterValues {
valuesBySchemaID[parameterValue.SchemaID.String()] = parameterValue
Expand Down
6 changes: 3 additions & 3 deletions provisionerd/provisionerd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ import (
"github.com/coder/retry"
)

// IsMissingParameterError returns whether the error is a missing parameter error.
// IsMissingParameterErrorCode returns whether the error is a missing parameter error.
// This can indicate to consumers that they should check parameters.
func IsMissingParameterError(errorCode string) bool {
return errorCode == runner.MissingParameterErrorCode
func IsMissingParameterErrorCode(code string) bool {
return code == runner.MissingParameterErrorCode
}

// Dialer represents the function to create a daemon client connection.
Expand Down
14 changes: 10 additions & 4 deletions provisionerd/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ const (
requiredTemplateVariablesErrorText = "required template variables"
)

var errorCodes = map[string]string{
MissingParameterErrorCode: missingParameterErrorText,
RequiredTemplateVariablesErrorCode: requiredTemplateVariablesErrorText,
}

var errUpdateSkipped = xerrors.New("update skipped; job complete or failed")

type Runner struct {
Expand Down Expand Up @@ -1058,10 +1063,11 @@ func (r *Runner) failedJobf(format string, args ...interface{}) *proto.FailedJob
message := fmt.Sprintf(format, args...)
var code string

if strings.Contains(message, missingParameterErrorText) {
code = MissingParameterErrorCode
} else if strings.Contains(message, requiredTemplateVariablesErrorText) {
code = RequiredTemplateVariablesErrorCode
for c, m := range errorCodes {
if strings.Contains(message, m) {
code = c
break
}
}
Copy link
Member

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?

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}.

Copy link
Member Author

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...

return &proto.FailedJob{
JobId: r.job.JobId,
Expand Down