Add more Pulp Exceptions.#4419
Conversation
aKlimau
force-pushed
the
add-pulp-exceptions
branch
5 times, most recently
from
f298692 to
4b371ec
Compare
aKlimau
force-pushed
the
add-pulp-exceptions
branch
5 times, most recently
from
fdcbcde to
18e72d8
Compare
aKlimau
force-pushed
the
add-pulp-exceptions
branch
from
18e72d8 to
fa0681b
Compare
Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
aKlimau
force-pushed
the
add-pulp-exceptions
branch
from
fa0681b to
f0a33c4
Compare
github-actions
Bot
added
multi-commit
| f"Failed to verify package signature: {completed_process.stdout} " | ||
| f"{completed_process.stderr}." | ||
| ) | ||
| raise PackageSignatureVerificationError(completed_process.stdout, completed_process.stderr) |
There was a problem hiding this comment.
Just a note to myself (not something for this PR, as this already existed before), maybe we can process known output errors so we don't have to pass the full stderr (and then possibly log the raw stderr).
| requested_checksum_type, ALLOWED_CHECKSUM_ERROR_MSG | ||
| ) | ||
| ) | ||
| raise DisallowedChecksumTypeError(requested_checksum_type, ALLOWED_CHECKSUM_ERROR_MSG) |
There was a problem hiding this comment.
ForbiddenChecksumTypeError and DisallowedChecksumTypeError looks like the same thing to me. Maybe we can pass down a context or extra message to the error for different contexts where it can be raised.
| signed_package_path = Path(result["rpm_package"]) | ||
| if not signed_package_path.exists(): | ||
| raise Exception(f"Signing script did not create the signed package: {result}") | ||
| raise SigningScriptError(result) |
There was a problem hiding this comment.
I think pulpcore InvalidSignatureError is appropriate here.
There was a problem hiding this comment.
Actually I think it's not quite the same. The one I mentioned is "Raised when a signature could not be verified.", but this workflow is about creating a package signature. Can you rename it to PackageSigningError ? Creation and verification are different enough
Another note to myself: maybe we can refine what is being passed here as well (e.g, the filename and fingerprint, instead of the signing script output, which can throw out anything).
| if os.stat(signature_file_path).st_size == 0: | ||
| log.error(f"{signature_file_path} is 0 bytes! sign_results: {sign_results}") | ||
| raise Exception("Signature file is 0 bytes") | ||
| raise SignatureFileEmptyError() |
There was a problem hiding this comment.
This is similar to the package signing error, but it's rpm metadata. Can you rename it to MetadataSigningError ?
| "Please check your ACS remote configuration." | ||
| ).format(custom_url, exc.status, exc.message) | ||
| ) | ||
| raise RemoteFetchError(custom_url, exc.status, exc.message) |
There was a problem hiding this comment.
The information about this being an ACS remote error is relevant to the user here. Maybe we can use another exception for ACS (a subclass of this? i dont know), or just pass that info to the the message argument. Also, for RemoteFetchError, I would also pass the remote name, which is not a context readily available in the task without doing extra hops.
|
|
||
| if not remote.url and not url: | ||
| raise ValueError(_("A remote must have a url specified to synchronize.")) | ||
| raise SyncError("A remote must have a url specified to synchronize.") |
There was a problem hiding this comment.
IHO this could be just a validation error, wdyt?
I don't even know when SyncError could be useful, since it's so vague. All errors raised here are running inside a task and will be available through the task object, right? So presumibly the user looking at this already knows it's looking at errors in an rpm sync.
There was a problem hiding this comment.
I think we can improve this changelog. Something like this (probably not exactly this): "Re-written generic exceptions as Pulp-specific exceptions, which represent known ways in which something can fail and are attached to specific error ids.". An if there is some documentation out there about this which might be relevant, it would be useful to link them here.
There was a problem hiding this comment.
I see exc.message is safe, nice!
https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientResponseError.message
| # can't be flagged as 'modular' thus broken repository! | ||
| if modulemd_result.url.endswith("zck"): | ||
| raise TypeError(_("Modular data compressed with ZCK is not supported.")) | ||
| raise UnsupportedZckCompressionError() |
There was a problem hiding this comment.
I feel this should be UnsupportedModularCompressionError and zck be an argument.
There was a problem hiding this comment.
It's a bit brittle to review and read this direct error code matching on tests.
Can you use the pattern ClassError.error_code, instead of hard-coding the codes? E.g MissingPrimaryMetadataError.error_code in exc.value.task.error["description"]
| f"Checksum {checksum} is unknown or too short to use for " | ||
| f"{self.publication.layout} publishing." | ||
| ) | ||
| raise ChecksumTooShortError(checksum, self.publication.layout) |
There was a problem hiding this comment.
What do you think about transforming this into a LayoutValidationError, which accepts layout and known validation causes (e.g short_checksum=checksum for this case)? Then the exception class can be responsible for producing the appropriate message if other layout validation errors are introduced. I'm not 100% sure this is a good pattern, but this error feels soo much specific (and it only makes sense to the nested-by-digest layout, btw).
2 participants
Assisted-by: Claude Sonnet 4.5 noreply@anthropic.com
📜 Checklist
See: Pull Request Walkthrough