Better error messages / error codes #5
Conversation
|
Blocked by oVirt/ovirt-engine-sdk-go#227. |
ghost
marked this pull request as ready for review
|
@Gal-Zaidman here's another one for review please. |
| case errors.As(err, &authErr): | ||
| fallthrough | ||
| case strings.Contains(err.Error(), "access_denied"): | ||
| return wrap(err, EAccessDenied, "access denied, check your credentials") |
There was a problem hiding this comment.
We have to test how the engine reacts in that case.
There was a problem hiding this comment.
Do you want to track it as an issue?
ghost
assigned
| ETimeout, | ||
| "timeout while removing disk", | ||
| ) | ||
| case <-time.After(10 * time.Second): |
There was a problem hiding this comment.
I see that the timeouts here are different on each retry method.
Maybe we should give the user a way to configure that with With.Context[1], and set a default.
I just think it is confusing that each retry method has a different interval (it also make sense because you don't want to retry connection like disk removal)
There was a problem hiding this comment.
@Gal-Zaidman leave it for now, this is handled properly in #14
client_template_get.go
Outdated
| } | ||
| template, err := convertSDKTemplate(sdkTemplate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert template object (%w)", err) | ||
| return nil, wrap(err, EUnidentified, "failed to convert template object") |
There was a problem hiding this comment.
Same as above, should probably be EBug
client_util.go
Outdated
| ovirtsdk "github.com/ovirt/go-ovirt" | ||
| ) | ||
|
|
||
| // waitForJobFinished waits for a job to truly finish. This is especially important when disks are involved as their status changes to OK prematurely. |
There was a problem hiding this comment.
I think you can remove "This is especially important when disks are involved as their status changes to OK prematurely." or at least say "Note: This is especially important when disks are involved as their status changes to OK prematurely."
Since it doesn't describe the function, just the motivation for creating/using it
There was a problem hiding this comment.
Also maybe add an example of how to set a correlationID, since it is not trivial, something like:
// waitForJobFinished waits for a job to truly finish by checking it was removed from the oVirt job queue using a caller predefined job correlationID.
// correlationID is a quarry parameter assigned to a job before it is sent to the ovirt engine, it must be unique and under 30 chars **(I think its a bit more but to be on the safe side).
// To set a correlationID add `Query("correlation_id", correlationID)` to the engine API call, for example:
correlationID := fmt.Sprintf("image_transfer_%s", utilrand.String(5))
conn.SystemService().DisksService().DiskService(diskId).
Update().
Query("correlation_id", correlationID).
Send()
| @@ -19,13 +18,13 @@ type VMClient interface { | |||
| // is 0. If the parameters are guaranteed to be non-zero MustNewVMCPUTopo should be used. | |||
| func NewVMCPUTopo(cores uint, threads uint, sockets uint) (VMCPUTopo, error) { | |||
There was a problem hiding this comment.
Why are you panic in https://github.com/oVirt/go-ovirt-client/pull/5/files#diff-c9e88235f1f664443d76481488dac388b8a048485d63e1c41d244add41d5940aR41
I think an error should be returned
There was a problem hiding this comment.
I'm afraid this link is broken. Can you add the comment in the correct place?
There was a problem hiding this comment.
- Some very minor changes are required.
- Wanted to ask you to implement a general retry function since it has the same structure everywhere, but I see you beat me to the punch Retry #14
Please describe the change you are making
This PR adds better error messages and error codes to identify failures from the oVirt engine.