From 3994cb7a893c8a24c78a9576e16ee080fdc63300 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Mar 2022 10:42:16 +0100 Subject: [PATCH 1/3] Build: `RepositoryError` message When a `RepositoryError` _without_ a message was raised we were setting the message as `"None"` and reporting this to the user :( This commit first checks if the exception already contains a message and use it in that case. If the exception does not contains a message, it raises it without a message so the generic one is added by the Celery handler (`on_failure`). Also, it improves the messages reported to the user to communicate a more detailed error depending the case. --- readthedocs/projects/exceptions.py | 12 ++++++++++-- readthedocs/vcs_support/backends/bzr.py | 6 ++++-- readthedocs/vcs_support/backends/git.py | 12 +++++++----- readthedocs/vcs_support/backends/hg.py | 8 +++++--- readthedocs/vcs_support/backends/svn.py | 3 +-- readthedocs/vcs_support/base.py | 10 ++++++---- 6 files changed, 33 insertions(+), 18 deletions(-) diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index bf68193715a..e2f948581ac 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- """Project exceptions.""" @@ -47,7 +46,16 @@ class RepositoryError(BuildUserError): FAILED_TO_CHECKOUT = _('Failed to checkout revision: {}') - def get_default_message(self): + GENERIC_ERROR = _( + "There was a problem with your repository. " + "Please, take a look at the commands' output to find out the reason.", + ) + + @property + def CLONE_ERROR(self): if settings.ALLOW_PRIVATE_REPOS: return self.PRIVATE_ALLOWED return self.PRIVATE_NOT_ALLOWED + + def get_default_message(self): + return self.GENERIC_ERROR diff --git a/readthedocs/vcs_support/backends/bzr.py b/readthedocs/vcs_support/backends/bzr.py index b4f73b3be9d..2ccfdd1be8b 100644 --- a/readthedocs/vcs_support/backends/bzr.py +++ b/readthedocs/vcs_support/backends/bzr.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- """Bazaar-related utilities.""" @@ -36,7 +35,10 @@ def up(self): def clone(self): self.make_clean_working_dir() - self.run('bzr', 'checkout', self.repo_url, '.') + try: + self.run("bzr", "checkout", self.repo_url, ".") + except RepositoryError: + raise RepositoryError(RepositoryError.CLONE_ERROR) @property def tags(self): diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 2fd0da99f3c..826e7d38211 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -1,12 +1,12 @@ """Git-related utilities.""" -import structlog import re import git -from gitdb.util import hex_to_bin +import structlog from django.core.exceptions import ValidationError from git.exc import BadName, InvalidGitRepositoryError, NoSuchPathError +from gitdb.util import hex_to_bin from readthedocs.builds.constants import EXTERNAL from readthedocs.config import ALL @@ -20,7 +20,6 @@ from readthedocs.projects.validators import validate_submodule_url from readthedocs.vcs_support.base import BaseVCS, VCSVersion - log = structlog.get_logger(__name__) @@ -197,8 +196,11 @@ def clone(self): cmd.extend([self.repo_url, '.']) - code, stdout, stderr = self.run(*cmd) - return code, stdout, stderr + try: + code, stdout, stderr = self.run(*cmd) + return code, stdout, stderr + except RepositoryError: + raise RepositoryError(RepositoryError.CLONE_ERROR) @property def lsremote(self): diff --git a/readthedocs/vcs_support/backends/hg.py b/readthedocs/vcs_support/backends/hg.py index 670e1423ee7..e37cb738fe6 100644 --- a/readthedocs/vcs_support/backends/hg.py +++ b/readthedocs/vcs_support/backends/hg.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- """Mercurial-related utilities.""" from readthedocs.projects.exceptions import RepositoryError @@ -33,8 +32,11 @@ def pull(self): def clone(self): self.make_clean_working_dir() - output = self.run('hg', 'clone', self.repo_url, '.') - return output + try: + output = self.run("hg", "clone", self.repo_url, ".") + return output + except RepositoryError: + raise RepositoryError(RepositoryError.CLONE_ERROR) @property def branches(self): diff --git a/readthedocs/vcs_support/backends/svn.py b/readthedocs/vcs_support/backends/svn.py index 03204ca1d9b..7059451fbcf 100644 --- a/readthedocs/vcs_support/backends/svn.py +++ b/readthedocs/vcs_support/backends/svn.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- """Subversion-related utilities.""" @@ -63,7 +62,7 @@ def co(self, identifier=None): url = self.repo_url retcode, out, err = self.run('svn', 'checkout', url, '.') if retcode != 0: - raise RepositoryError + raise RepositoryError(RepositoryError.CLONE_ERROR) return retcode, out, err @property diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 51bd001906d..8e7be0b2627 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -1,11 +1,11 @@ """Base classes for VCS backends.""" -import structlog import os import shutil -from readthedocs.doc_builder.exceptions import BuildUserError, BuildCancelled -from readthedocs.projects.exceptions import RepositoryError +import structlog +from readthedocs.doc_builder.exceptions import BuildCancelled, BuildUserError +from readthedocs.projects.exceptions import RepositoryError log = structlog.get_logger(__name__) @@ -108,7 +108,9 @@ def run(self, *cmd, **kwargs): raise BuildCancelled except BuildUserError as e: # Re raise as RepositoryError to handle it properly from outside - raise RepositoryError(str(e)) + if hasattr(e, "message"): + raise RepositoryError(e.message) + raise RepositoryError # Return a tuple to keep compatibility return (build_cmd.exit_code, build_cmd.output, build_cmd.error) From 434275444ac33ad16d883f42f2c9d585eed59928 Mon Sep 17 00:00:00 2001 From: Anthony Date: Mon, 7 Mar 2022 17:03:06 -0700 Subject: [PATCH 2/3] Update readthedocs/projects/exceptions.py --- readthedocs/projects/exceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index e2f948581ac..dd59feac5f6 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -47,8 +47,8 @@ class RepositoryError(BuildUserError): FAILED_TO_CHECKOUT = _('Failed to checkout revision: {}') GENERIC_ERROR = _( - "There was a problem with your repository. " - "Please, take a look at the commands' output to find out the reason.", + "There was a problem cloning your repository. " + "Please check the command output for more information.", ) @property From 6e69997c8845d24f26b849abe22abd82c74c8392 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Mon, 7 Mar 2022 16:12:31 -0800 Subject: [PATCH 3/3] Ignore property method upper case naming --- readthedocs/projects/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/exceptions.py b/readthedocs/projects/exceptions.py index dd59feac5f6..489ab6a9ede 100644 --- a/readthedocs/projects/exceptions.py +++ b/readthedocs/projects/exceptions.py @@ -52,7 +52,7 @@ class RepositoryError(BuildUserError): ) @property - def CLONE_ERROR(self): + def CLONE_ERROR(self): # noqa: N802 if settings.ALLOW_PRIVATE_REPOS: return self.PRIVATE_ALLOWED return self.PRIVATE_NOT_ALLOWED