-
Notifications
You must be signed in to change notification settings - Fork 24
Fix get_vms unhandled exception #69
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
Conversation
Add None check if no VMs are present in resources list. This allows an empty array to be successfully returned instead of causing an exception.
|
@ericbram i would think that you would want to action to fail if it failed to find the VM? |
|
In my case, I'm getting a list of VMs in a cluster. It makes sense to me to return an empty list if there are no VMs present, as that's representative of the truth. If there are cases where it's good to fail the action, I think that's understandable, but I don't think the correct action is to fail due to a null exception |
|
I know right now we use Maybe you could add a flag like Just don't want to break a bunch of existing workflows for current users. |
|
I think that's a fair point, is that the line where you actually get your failure? I was just looking for a quick fix to solve the issue in my case, but it seems like this function is overloaded and I don't have a pressing need to rework it. I'll just make note that the exception I see means no VMs are found, and not that our configuration is wrong. Thanks! |
|
I know for us, we don't look at the error message. We assume error = VM not found. I don't mind a change here, you could totally add a flag in if you would like different default behavior. I certainly see the value of that change and would appreciate the contribution! |
nmaludy
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.
Would like to see this different behavior turned into a flag, if possible.
Please also update the CHANGELOG entry and increment the pack's version by 0.1.0 (added new feature).
Thanks!
|
@nmaludy I'm really sorry about this, I started using my work account permanently and forgot I did this PR. I updated with your requests, please let me know if you'd like to see any other changes. |
|
@jschoewe @bishopbm1 thoughts? |
|
@nmaludy @ericbram I definitely see the use case for this, I tested it in our environment and it looks good to me. Something I just noticed when testing this, though, is that if we provide a cluster with no VMs in it then it fails and if we provide an moid that doesn't exist in the "ids" field then it also fails, but if we provide a name that doesn't exist in the "names" field then the action succeeds and returns an empty array. I'm thinking this should fail too if the name isn't found but I don't want to mess up anyone's current workflows so I'm fine leaving it how it is if you are. |
bishopbm1
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.
LGTM!
|
|
Add None check if no VMs are present in resources list. This allows an empty array to be successfully returned instead of causing an exception.