Build: refactor logic to validate artifacts output paths #9940
Conversation
Wrap all the logic into a class method so it's easier to follow and read. It also remove some duplicated checks of `os.path.exists`, making sure we are always using _validated_ paths already.
humitos
requested review from
ericholscher
and removed request for
benjaoming
| # Check if there are multiple files on artifact directories. | ||
| # These output format does not support multiple files yet. | ||
| # In case multiple files are found, the upload for this format is not performed. | ||
| # | ||
| # TODO: we should fail the build for these cases and clearly communicate this. | ||
| # to the user. To do this, we need to call this method (``store_build_artifacts``) | ||
| # since the ``execute`` method. | ||
| # It will allow us to just `raise BuildUserError` and handle it at | ||
| # Celery `on_failure` handler. |
There was a problem hiding this comment.
| # Check if there are multiple files on artifact directories. | |
| # These output format does not support multiple files yet. | |
| # In case multiple files are found, the upload for this format is not performed. | |
| # | |
| # TODO: we should fail the build for these cases and clearly communicate this. | |
| # to the user. To do this, we need to call this method (``store_build_artifacts``) | |
| # since the ``execute`` method. | |
| # It will allow us to just `raise BuildUserError` and handle it at | |
| # Celery `on_failure` handler. | |
| # Check if there are multiple files in artifact directories. | |
| # These output formats don't support multiple files yet. | |
| # In case multiple files are found, the upload for this format is not performed. | |
| # | |
| # TODO: we should fail the build for these cases and clearly communicate this | |
| # to the user. To do this, we need to call this method (``store_build_artifacts``) | |
| # It will allow us to just `raise BuildUserError` and handle it at | |
| # in the `on_failure` handler. |
Wasn't really sure what the second part here was saying with the since the ``execute`` method.?
There was a problem hiding this comment.
Meh, s/since/from inside/. So, the correct sentence there should be:
we should fail the build for these cases and clearly communicate this to the user. To do this, we need to call this method (
store_build_artifacts) from inside theexecutemethod.
There was a problem hiding this comment.
This comment was removed in the next iteration anyways, since we are calling store_build_artifacts from inside execute now and we can just raise exceptions there.
| # since the ``execute`` method. | ||
| # It will allow us to just `raise BuildUserError` and handle it at | ||
| # Celery `on_failure` handler. | ||
| if artifact_type in ("htmlzip", "epub", "pdf"): |
There was a problem hiding this comment.
Seems this should be using a constant?
|
I closed this PR. We are continuing the conversation in #9941. I'll address all the feedback you provided here in that PR. I'm sorry for all these confusions 🙏🏼 |
Wrap all the logic into a class method so it's easier to follow and read.
It also remove some duplicated checks of
os.path.exists,making sure we are always using validated paths already.
PR based in #9939 (we can probably close that one and only review this one)
Related #9931