From ca860b86cddc49713dc820df9d0ad3aad15489c3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 3 Dec 2024 13:31:07 -0500 Subject: [PATCH] Build: better error message when rclone fails (#11796) So, we were using incorrectly the exceptions, as we were passing a message to them, but they were expecting a message_id, I guess what we wanted is to have that error being show somewhere in the exception message, so I added a field for that, but the notification itself will show the generic error. And of course, there is now one message for rclone failures. closes https://github.com/readthedocs/readthedocs.org/issues/11544 --- readthedocs/doc_builder/environments.py | 25 +++++++++++++++++++------ readthedocs/doc_builder/exceptions.py | 1 + readthedocs/notifications/exceptions.py | 6 ++++-- readthedocs/notifications/messages.py | 13 +++++++++++++ readthedocs/projects/tasks/builds.py | 12 +++++++++--- 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 9dd49046981..0d618c5618c 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -80,7 +80,10 @@ def __init__( self.user = user or settings.RTD_DOCKER_USER self._environment = environment.copy() if environment else {} if "PATH" in self._environment: - raise BuildAppError("'PATH' can't be set. Use bin_path") + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, + exception_message="'PATH' can't be set. Use bin_path", + ) self.build_env = build_env self.output = None @@ -493,7 +496,10 @@ def run_command_class( if "bin_path" not in kwargs and env_path: kwargs["bin_path"] = env_path if "environment" in kwargs: - raise BuildAppError("environment can't be passed in via commands.") + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, + exception_message="environment can't be passed in via commands.", + ) kwargs["environment"] = environment kwargs["build_env"] = self build_cmd = cls(cmd, **kwargs) @@ -616,7 +622,8 @@ def __enter__(self): if state is not None: if state.get("Running") is True: raise BuildAppError( - _( + BuildAppError.GENERIC_WITH_BUILD_ID, + exception_message=_( "A build environment is currently " "running for this version", ), @@ -629,7 +636,9 @@ def __enter__(self): client = self.get_client() client.remove_container(self.container_id) except (DockerAPIError, ConnectionError) as exc: - raise BuildAppError(exc.explanation) from exc + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, exception_message=exc.explanation + ) from exc # Create the checkout path if it doesn't exist to avoid Docker creation if not os.path.exists(self.project.doc_path): @@ -703,7 +712,9 @@ def get_client(self): ) return self.client except DockerException as exc: - raise BuildAppError(exc.explanation) from exc + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, exception_message=exc.explanation + ) from exc def _get_binds(self): """ @@ -816,4 +827,6 @@ def create_container(self): ) client.start(container=self.container_id) except (DockerAPIError, ConnectionError) as exc: - raise BuildAppError(exc.explanation) from exc + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, exception_messag=exc.explanation + ) from exc diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index eb934fdb80f..960167351ea 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -24,6 +24,7 @@ class BuildAppError(BuildBaseException): default_message = _("Build application exception") GENERIC_WITH_BUILD_ID = "build:app:generic-with-build-id" + UPLOAD_FAILED = "build:app:upload-failed" BUILDS_DISABLED = "build:app:project-builds-disabled" BUILD_DOCKER_UNKNOWN_ERROR = "build:app:docker:unknown-error" BUILD_TERMINATED_DUE_INACTIVITY = "build:app:terminated-due-inactivity" diff --git a/readthedocs/notifications/exceptions.py b/readthedocs/notifications/exceptions.py index be9b7833eea..a14ade79ecc 100644 --- a/readthedocs/notifications/exceptions.py +++ b/readthedocs/notifications/exceptions.py @@ -12,7 +12,9 @@ class NotificationBaseException(Exception): default_message = _("Undefined error") - def __init__(self, message_id, format_values=None, **kwargs): + def __init__( + self, message_id, format_values=None, exception_message=None, **kwargs + ): self.message_id = message_id self.format_values = format_values - super().__init__(self.default_message, **kwargs) + super().__init__(exception_message or self.default_message, **kwargs) diff --git a/readthedocs/notifications/messages.py b/readthedocs/notifications/messages.py index 8c5e0049538..0d0c471195c 100644 --- a/readthedocs/notifications/messages.py +++ b/readthedocs/notifications/messages.py @@ -96,6 +96,19 @@ def get_rendered_body(self): ), type=ERROR, ), + Message( + id=BuildAppError.UPLOAD_FAILED, + header=_("There was a problem while updating your documentation"), + body=_( + textwrap.dedent( + """ + Make sure this project is outputting files to the correct directory, or try again later. + If this problem persists, report this error to us with your build id ({{ instance.pk }}). + """ + ).strip(), + ), + type=ERROR, + ), Message( id=BuildAppError.BUILD_TERMINATED_DUE_INACTIVITY, header=_("Build terminated due to inactivity"), diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 156c3988b7a..bc093e714bd 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -479,7 +479,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): # Known errors in our application code (e.g. we couldn't connect to # Docker API). Report a generic message to the user. if isinstance(exc, BuildAppError): - message_id = BuildAppError.GENERIC_WITH_BUILD_ID + message_id = exc.message_id # Known errors in the user's project (e.g. invalid config file, invalid # repository, command failed, etc). Report the error back to the user @@ -951,7 +951,10 @@ def store_build_artifacts(self): # 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.") from exc + raise BuildAppError( + BuildAppError.UPLOAD_FAILED, + exception_message="Error uploading files to the storage.", + ) from exc # Delete formats for media_type in types_to_delete: @@ -973,7 +976,10 @@ def store_build_artifacts(self): # 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 deleting files from storage.") from exc + raise BuildAppError( + BuildAppError.GENERIC_WITH_BUILD_ID, + exception_message="Error deleting files from storage.", + ) from exc log.info( "Store build artifacts finished.",