I'm thinking about extending convertFault a small bit. If faultCode is present it should almost always come from koji. In such a case I would say it is +- safe to convert it to GenericError instead with the following code. There are probably some extreme cases where it could be misleading (faultCode is not from koji but from the underlying layer of xmlrpclib), but it should be safe in the most cases. Another improvement could be that faultCode range could be part of e.g. getKojiVersion(extended=True) which could return dict with version itself, coderange and maybe some additional info (like plugins enabled at hub).

About the importance - I think that the biggest problem wouldn't be the CLI - it is easy to see that it failed, but automation scripts and possible plugins. Their current GenericError handlers will stop working and just propagate exceptions further. So, wrapping everything as GenericError seems like a safer way to me.

Another options is the original one: Introduce new Exception ASAP but start using them in a ~year. I don't think it is that much helping. I see that (client) deployments are mostly in two categories - they don't update almost at all (old isolated boxes) and those which are updating for almost every release. It would help us in those small time windows (fedora upgraded koji hub, but users still don't have actual systems). It is still worth, but it can be reduced to +1 release (introduce exception in 1.27 and start them using in 1.28).

Mike, any thoughts?

for v in globals().values(): if isinstance(v, type(Exception)) and issubclass(v, GenericError) and \ code == getattr(v, 'faultCode', None): ret = v(fault.faultString) ret.fromFault = True return ret + else: + raise GenericError(fault.faultString)

On Thu, Oct 21, 2021 at 6:17 PM Ken Dreyer ktdreyer@ktdreyer.com wrote:

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

---

koji-devel mailing list -- koji-devel@lists.fedorahosted.org To unsubscribe send an email to koji-devel-leave@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/koji-devel@lists.fedorahosted.o... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure