Skip to content

Commit

Permalink
Build: better error message when rclone fails (#11796)
Browse files Browse the repository at this point in the history
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 #11544
  • Loading branch information
stsewd authored Dec 3, 2024
1 parent 0d7643a commit ca860b8
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 11 deletions.
25 changes: 19 additions & 6 deletions readthedocs/doc_builder/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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",
),
Expand All @@ -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):
Expand Down Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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
1 change: 1 addition & 0 deletions readthedocs/doc_builder/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 4 additions & 2 deletions readthedocs/notifications/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
13 changes: 13 additions & 0 deletions readthedocs/notifications/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
12 changes: 9 additions & 3 deletions readthedocs/projects/tasks/builds.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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.",
Expand Down

0 comments on commit ca860b8

Please sign in to comment.