Repository navigation
SharedFileSystems: apiversions package - #409
Conversation
jtopjian
changed the title
|
LGTM. |
|
@jrperritt I'll appreciate if you have a look at this PR as there are two other PRs #346 and #381 that depend on this PR. |
|
@jtopjian is it possible to merge this PR or do you prefer to wait for review from @jrperritt |
Using types for Errors. Added erroneous API results to validate errors.
|
Implemented Error types to better conform to how other errors are handled. |
| // could not be found. | ||
| type ErrVersionNotFound struct{} | ||
|
|
||
| func (e ErrVersionNotFound) Error() string { |
There was a problem hiding this comment.
There is a kind of framework for errors in the gophercloud library.
This framework is used for defining errors, e.g. in https://github.com/gophercloud/gophercloud/blob/master/openstack/errors.go
However, the framework is not used here so it feels inconsistent.
There was a problem hiding this comment.
There is a kind of framework for errors in the gophercloud library.
I am aware of this.
However, the framework is not used here so it feels inconsistent.
Can you provide a more concrete example of how it can be used? I did not see an existing error type that would best fit the errors defined here and I didn't want to use BaseError just to say I used BaseError.
There was a problem hiding this comment.
I agree that there is no existing error that would fit.
My point is that BaseError and func choseErrString are not used. However, I understand that you don't see much benefit using BaseError and func choseErrString.
jrperritt
left a comment
jrperritt
left a comment
There was a problem hiding this comment.
This looks fine. There is already an issue for changing how errors are dealt with.
* vendor: Updating Gophercloud * Image Service v2: Support Updating Properties This commit enables full addition, modification, and removal of properties during an image update. In addition, the "direct_url" property will automatically be added to the list of all properties if it is detected. * Addressing review comments
5 participants
For #408
@jrperritt There's some discussion here:
jtopjian@5bf58ab
jtopjian@7f48e23
This implementation could definitely use some critique when you have time.
Should proper error types be created?
The most notable oddity is that when retrieving a major version, an array is returned. I can't see why more than one result would be returned, but is that a safe assumption?
If it's best to handle multiple results, then this gets a little weird. The way I see it, a Get should be treated like a List and there should then be some convenience functions to extract the best result (or specify the type of result). But since there's no example of what multiple results would look like, it's really hard to think ahead.
/cc @pospispa