PR#632: Adding xmlrpc fault exception -

greenwave

#632 Adding xmlrpc fault exception

Merged May 14, 2021 by rlim. Opened May 10, 2021 by rlim.

rlim/greenwave 460_bogus_item_500 into master

rlim commented May 10, 2021

Jira Ticket: https://projects.engineering.redhat.com/browse/CWFHEALTH-460

Initially the scope of this task was to catch an error caused from an invalid subject identifier, preventing a status 500 error. As that change has already been implemented after the creation of this ticket, the new scope of this task is to add code to catch the specific error that is caused from the invalid identifier. Currently, when an invalid subject identifier is entered the user will receive a response of "unexpected error"; to give more transparency to the user for the error we want to return the specific reason.

Old Error Response
{
"error": "Unexpected error", <--( Old response )
"scenario": null,
"sources": [],
"subject_identifier": "nieco",
"subject_type": "koji_build",
"testcase": "failed-fetch-gating-yaml",
"type": "failed-fetch-gating-yaml"
},

New Error Response
{
"error": "Koji XMLRPC fault due to: 'invalid format: nieco'", <--( New Response )
"scenario": null,
"sources": [],
"subject_identifier": "nieco",
"subject_type": "koji_build",
"testcase": "failed-fetch-gating-yaml",
"type": "failed-fetch-gating-yaml"
},

Note: The "invalid format: nieco" is the full 'err.faultString' response from the xmlrpc.client.Fault Exception. The exception, "err", does not provide any fields to parse to extract that it is regarding the subject_identifier.

mprahl commented May 10, 2021

This is a very specific error message but you are catching a generic xlmrpc fault from Koji. Could you please ensure that the the fault is actually that the identifier is invalid and nothing else? If it's something else, then just reraise the exception for the generic catch block to handle it.

mprahl commented May 10, 2021

Could you please change the commit message to something that describes the change and its purpose rather than the describing the implementation details?

For example, you could have Provide a descriptive error when a Koji build identifier is an invalid format.

mprahl commented May 10, 2021

This is a nitpick but the rest of the code does it this way. The reason to perform logging this way is that if logging is disabled for this logger at this level, the string interpolation will not occur. The way you are doing it causes string interpolation to always be performed regardless of the logging configuration.

logging.exception('Unexpected Koji XMLRPC fault with code: %s', err.faultCode)

1 new commit added

Provide a descriptive error when a Koji build identifier is of an invalid format

May 10, 2021

lholecek commented May 11, 2021

Nice. Can you squash the two commits before we merge it?

BTW, commit message subject line should use imperative mood. Good guidelines for writing commit messages are here: https://chris.beams.io/posts/git-commit/

I will close the upstream issue #526 since it has been fixed elsewhere.

rebased onto 3c8da7747753d24ea716de556bad1a436a1af640

May 11, 2021

rlim commented May 11, 2021

Nice. Can you squash the two commits before we merge it?

BTW, commit message subject line should use imperative mood. Good guidelines for writing commit messages are here: https://chris.beams.io/posts/git-commit/

I will close the upstream issue #526 since it has been fixed elsewhere.

Commits have been squashed! And ahh I see, I'll follow this git commit guideline for my messages

Edited May 11, 2021 by rlim

rebased onto beb963cc608123611b07bf28fc3e52b963371c69

May 11, 2021

rlim commented May 11, 2021

Note: Ammended commit to add in the JIRA ticket

lholecek commented May 12, 2021

+1, good to merge if @mprahl agrees.

mprahl commented May 13, 2021

:thumbsup:

lholecek commented May 14, 2021

@rlim Feel free to merge the code (I've added you to the factory2 group so you should have the permissions).