-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
one_host: Fix ID logic #8907
one_host: Fix ID logic #8907
Conversation
self.result['changed'] = True | ||
except Exception as e: |
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.
Just to be on the safe side, have you checked whether the other if
statements you are modifying do not fall under the same situation?
Methods:
one.host.status()
one.host.delete()
one.host.update()
one.host.addhost()
Don't they also trigger exceptions when parameters are invalid?
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.
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.
So, in that case, wouldn't it be good to add the try-except
around them below as well?
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.
Could you please show which if statemets are wrong as well? From my perspective other if statements look right
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.
I would not go as far as saying they are wrong, but the same way that you transformed the if statement into a try-except, maybe those could benefit from that as well - line numbers on your branch (right hand side):
- line 229
- line 245
- line 259
- line 282
- line 291
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.
oops, I completely missed them, thanks!
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.
Beware that by using this, we are assuming that those calls will raise exceptions when failing (which makes sense, but not all developers write code that way). I would suggest you confirm that if you haven't yet.
Other than that, LGTM.
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.
yeah, it's too much IMO, no need to use it for every call :)
I simply do not use this module that much, so if one day I would need to use this module directly then might as well check if these are necessary
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
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
I did hit the button twice by mistake. 🤦 |
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #9014 🤖 @patchback |
* Fix one_host module * Add CHANGELOG fragment * PR Fixes * Update exceptions (cherry picked from commit 8df9d0d)
@abakanovskii thanks for your contribution! |
one_host: Fix ID logic (#8907) * Fix one_host module * Add CHANGELOG fragment * PR Fixes * Update exceptions (cherry picked from commit 8df9d0d) Co-authored-by: alexander <[email protected]>
SUMMARY
Fix if statements when ID equals to 0
Fixes #1199
ISSUE TYPE
COMPONENT NAME
one_host
ADDITIONAL INFORMATION
So I tested all of the
one.
commands and It seem to be returning ID (integer) of created/updated host or clusterSo using simple
if some_variable:
will not work if ID=0 because bool(0) = False and since and since OpenNebula starts ID counter from zero thats a big problem