On Thu, Oct 21, 2021 at 7:20 AM Tomas Kopecek tkopecek@redhat.com wrote:
I'm +1 here. I'm wondering (and we've had that discussion already somewhere) why we should wait with introducing new exceptions. If we just change GenericErrors to new ones we shouldn't break anything (older code will still catch GenericError descendants). And I think there was some good reason, Mike?
I actually tested this recently, raising a new custom exception in get_user(). My notes are in https://github.com/ktdreyer/koji-ansible/issues/221
The biggest general problem that I can see is that old clients's convertFault() method does not translate new (unknown) faults to a GenericError. It simply returns the xmlrpc.client.Fault as-is. In some cases, old clients have code like:
try: session.someRPC() except GenericError: # nice error handling here
Rather than catching all possible faults like:
try: session.someRPC() except (GenericError, six.moves.xmlrpc_client.Fault): # nice error handling here
Both patterns are present in koji's codebase. But the former pattern could lead to newly un-handled exceptions.
It's not clear to me how important this is. I haven't looked at every bit of code that catches GenericError yet. I *am* finding that a lot of CLI commands simply make RPCs without catching anything at all, so they just let any exceptions bubble up. For example, like I mentioned in that koji-ansible issue, the `koji set-pkg-owner` CLI has no try/except at all, and as a result, the UX does not differ very much. The error text looks a tiny bit different, but as long as we maintain a faultString on the hub that starts with "No such user", I think a human can still understand what Koji is saying.
- Ken