exoscale / cs

A simple, yet powerful CloudStack API client for python and the command-line.
BSD 3-Clause "New" or "Revised" License
88 stars 36 forks source link

Include errortext to provide transparent information about backend errors #111

Closed synergiator closed 4 years ago

synergiator commented 4 years ago

Addresses #110

greut commented 4 years ago

It looks nice. Although, I'd put that logic where the exception is shown to the user.

synergiator commented 4 years ago

thank you @greut - how do you mean, "put that logic where the exception is shown to the user"? Do you mean around the line 288?

 if self.trace:
                print(response.status_code, response.reason, file=sys.stderr)
greut commented 4 years ago

@1605200517 cs is both a library and a client. If you're using the library, you'll have to do it yourself, when using the client (cli), that piece of code should have you covered.

https://github.com/exoscale/cs/blob/master/cs/__init__.py#L106-L121

greut commented 4 years ago

@1605200517 the trace option is more a "full debug" mode flag than an error handling solution.

synergiator commented 4 years ago

@greut thank you for your advice!

Don't understand the sentence "If you're using the library, you'll have to do it yourself". Why not have explicit error message in any case?

Let's have a look at the following example. We call CS API with a wrong argument and get a HTTP 431. As far I understand I use cs here as a library if I use the import statement and work with the cs object.

import cs
...
cs.listTemplates() # missing mandatory templatefilter argument

On execution of this code then, there is the following exception stack trace over the lines 213, 295 and 357 of client.py.

Traceback (most recent call last):
  File "testcsfix.py", line 5, in <module>
    cs.listTemplates()
  File "/home/user/.virtualenvs/csfix/lib/python3.6/site-packages/cs/client.py", line 213, in handler
    return self._request(command, **kwargs)
  File "/home/user/.virtualenvs/csfix/lib/python3.6/site-packages/cs/client.py", line 295, in _request
    data = self._response_value(response, json)
  File "/home/user/.virtualenvs/csfix/lib/python3.6/site-packages/cs/client.py", line 357, in _response_value
    response=response)
cs.client.CloudStackApiException: HTTP 431 response from CloudStack

The exception message is: HTTP 431 response from CloudStack. So the important debugging information about the actual error is missing.

I do not understand therefore the above code reference to init.py as it does not appear in the exception stack trace and what should be changed in my pull request.

That is, custom code to handle this would mean to introduce cs call wrapper every time you make a call. To do so for every API call just to guarantee I don't miss the information about the actual problem looks to me like too much boilerplate code.

result = None
try:
    result = cs.listTemplates()

except Exception as e:
    ddata = e.response.json()
    k,val = ddata.popitem()
    status_code = e.error
    error_message = "HTTP {0} response from CloudStack.\nErrorcode {1}: {2}"
    fmt = error_message.format(e.error['errorcode'], e.error['cserrorcode'], e.error['errortext'])
    raise Exception(fmt)

# proceed with the API call result
greut commented 4 years ago

The exception "message" is most of the tip the tip of the iceberg. E.g. when using the requests library, you'll have to dig the request and response argument as needed.

The .response is not part of Exception, hence you're breaking inheritance by assuming so.

When using cs as a library, hence not a a command line interface program, you're free to handle the exception they it pleases you.

synergiator commented 4 years ago

I still miss the point why one would expect a user of a library to dig into the facade iceberg just to get a useful error message relevant to the actual system in use - Apache Cloud Stack.