-
Notifications
You must be signed in to change notification settings - Fork 136
Add more useful PulpExceptions for general tasks #7271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b484ef7 to
b015f23
Compare
pulpcore/exceptions/base.py
Outdated
| if error_code: | ||
| if not isinstance(error_code, str): | ||
| raise TypeError(_("Error code must be an instance of str.")) | ||
| self.error_code = error_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an abstract base class. So this seems overly complex.
We should be able to go with
def __init__(self):
if not isinstance(self.error_code, str):
raise NotImplementedError("ABC error. Subclass must define a unique error_code.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could think about making this capable of providing on-the-fly defined errors, but I think that would not be helping with the goal to get more consistent errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's partly why I made the class so complicated to keep the idea of defining on-the-fly errors, but it does go against trying to be more consistent.
pulpcore/exceptions/base.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe now that we are there:
return {"code": exc.error_code, "description": str(exc)}
This should help to make it machine readable.
5892910 to
33b9695
Compare
fixes: pulp#7270 Assisted-by: claude-4.5-sonnet
After looking through the plugins, most exceptions are some Runtime or ValueError and I'm not sure if there is an easy generic we can put into Pulpcore to cover all. Many of them are in our big tasks like sync/publish so I created two catch-alls for Sync & Publish. I was thinking of creating a specific error for Download failures, but our download logic is too convoluted to find a good place, and all the plugins do their own thing to check if a download failed. One big exception I didn't bother with is RBAC hooks. If they fail on object creation then the task won't show useful info, but this should only happen if the user customizes their access policies incorrectly.
fixes: #7270
Assisted-by: claude-4.5-sonnet
https://issues.redhat.com/browse/PULP-1171
📜 Checklist
See: Pull Request Walkthrough