Build: communicate uploading errors to users #9941
Conversation
It checks for `_readthedocs/<format>` output directory to be a directory. If it's not a directory, it silently skip that format for now. This behavior will change once we move out `store_build_artifacts` from `on_success` and we can communicate errors easily to the user. Related #9931
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.
Running `store_build_artifacts` from inside `.execute()` method instead of from `.on_sucess()` allow us to raise any exception and handle it properly. We can raise a `BuildUserError` to clearly communicate the specific error to the user, or any other exception to show the generic message: "Try again or contact us". Besides, this allows us to *fail the build* instead of continuing silently without the user noticing this: - there more than 1 file of formats that don't support multiple files - the output path is not a directory - there were errors uploading the artifacts to the storage
humitos
force-pushed
the
humitos/store-build-artifacts-execute
branch
from
2f6b59f to
00007ed
Compare
In case the `_readthedocs/<format>` directory is created by it contains 0 files, we fail the build and communicate this to the user.
| # Re-raise the exception to fail the build and handle it | ||
| # automatically at `on_failure`. | ||
| # It will clearly communicate the error to the user. | ||
| raise BuildAppError("Error uploading files to the storage.") |
There was a problem hiding this comment.
I feel like we need more information here. Won't part of the users files already be uploaded, but not all of them? We likely need to make this atomic somehow...
There was a problem hiding this comment.
Yes. I think atomic uploads is the correct way. However, I'm not sure how to do that in a simple way. I suppose we could upload the content to one folder and them move it to the final one, in case the "move to the final one" action is atomic. Otherwise, we are in the same situation.
|
I like the idea of atomic uploads and I think we should go in that direction. I can create an issue to discuss it a little more in deep and decide what path to follow. I'd like to clarify if you are OK merging this PR as-is, meaning that "if there is an error in the upload step, we fail the build and communicate this to the user --leaving half of the files updated in production" or, you prefer to come back to the previous behavior that was "if there is an error, continue the build, don't communicate to the user and leave half of the files updated in production". Basically, it's the same, but in the new one we inform the user about this. |
readthedocs/projects/tasks/builds.py
Outdated
| # 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. | ||
| if artifact_type in ("htmlzip", "epub", "pdf"): |
There was a problem hiding this comment.
Address Eric's feedback for this #9940 (comment)
I opened #9947 to track this and discuss there. I'm pinging @stsewd since he has been working with |
|
We are not at "atomic uploads" yet, but we will be retrying at least several times before failing the build with |
These files are required by `store_build_artifacts` to detect there are valid output on those folders.
Running
store_build_artifactsfrom inside.execute()methodinstead of from
.on_sucess()allow us to raise any exception and handle itproperly.
We can raise a
BuildUserErrorto clearly communicate the specific error to theuser, or any other exception to show the generic message: "Try again or contact
us".
Besides, this allows us to fail the build instead of continuing silently
without the user noticing this:
PR based in #9940
Related to #9931