Cluster shutdown module exception handling update #210
Conversation
justinc1
left a comment
justinc1
left a comment
There was a problem hiding this comment.
Change is ok, but more such changes are needed.
plugins/module_utils/cluster.py
Outdated
| # raise errors.ScaleComputingError(f"Request timed out: {e}") | ||
| except ScaleComputingError as e: | ||
| # To avoid timeout when there are a lot of VMs to shutdown | ||
| if "Request timed out" in str(e): |
There was a problem hiding this comment.
Here we also detect exception root cause from string, not from type. Are the more places with this pattern?
We should have a dedicated exception, like ScaleRequestTimedOut. Then you refactor the RestClient, and you need to check if there is any other module that needs to know about your new exception.
There was a problem hiding this comment.
But this error will never cause a problem right? The string is generated inside the client, so the string comparison will never fail due to formatting (like it would when we compared the string directly from the http response - in case of "connection refused")
There was a problem hiding this comment.
As discussed, I implemented ScaleTimeoutError in rest_client
PolonaM
force-pushed
the
module_exception_handling
branch
from
e988bae to
1f387ef
Compare
Use ConnectionRefusedError in cluster shutdown instead of error description comparison
Closes #137