-
Notifications
You must be signed in to change notification settings - Fork 55
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
openstack/clientconfig: fix dropped errors #196
Conversation
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.
Thank you for this fix.
@@ -330,7 +330,7 @@ func GetCloudFromYAML(opts *ClientOpts) (*Cloud, error) { | |||
|
|||
cloud.Interface = "" | |||
|
|||
return cloud, nil | |||
return cloud, err |
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 catching this. Can you please handle this error right where it's produced (line 312), possibly with an additional error message like we do on line 291?
@@ -46,6 +46,9 @@ func mergeClouds(override, cloud interface{}) (*Cloud, error) { | |||
var mergedCloud Cloud | |||
mergedInterface := mergeInterfaces(overrideInterface, cloudInterface) | |||
mergedJson, err := json.Marshal(mergedInterface) | |||
if err != nil { |
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.
👍
If you still want to work on this, I think we should have some test coverage of the change you propose. |
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.
OK let's merge what we have, which is worthwhile. Thanks for this @alrs
This picks up two dropped
err
variables inopenstack/clientconfig
.