From f9454da5e7960d0410edad28fe22ef4e16bbbb92 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Thu, 31 Dec 2020 23:57:10 +0100 Subject: [PATCH] [mod] trash this UX madness that is error_see_logs and improve error messages --- locales/en.json | 1 - moulinette/actionsmap.py | 31 ++++++++++++++++++------------- moulinette/interfaces/__init__.py | 30 ++++++++++++++++-------------- moulinette/interfaces/api.py | 26 ++++++++++++++++++-------- moulinette/interfaces/cli.py | 20 ++++++++++++++------ test/conftest.py | 2 +- test/test_actionsmap.py | 4 ++-- 7 files changed, 69 insertions(+), 45 deletions(-) diff --git a/locales/en.json b/locales/en.json index 7558e910..fbc49a55 100644 --- a/locales/en.json +++ b/locales/en.json @@ -7,7 +7,6 @@ "deprecated_command": "'{prog} {command}' is deprecated and will be removed in the future", "deprecated_command_alias": "'{prog} {old}' is deprecated and will be removed in the future, use '{prog} {new}' instead", "error": "Error:", - "error_see_log": "An error occurred. Please see the logs for details, they are located in /var/log/yunohost/.", "file_exists": "File already exists: '{path}'", "file_not_exist": "File does not exist: '{path}'", "folder_exists": "Folder already exists: '{path}'", diff --git a/moulinette/actionsmap.py b/moulinette/actionsmap.py index bf5db8eb..7b7dc53f 100644 --- a/moulinette/actionsmap.py +++ b/moulinette/actionsmap.py @@ -304,14 +304,12 @@ class ExtraArgumentParser(object): # Validate parameter value parameters[p] = klass.validate(v, arg_name) except Exception as e: - logger.error( - "unable to validate extra parameter '%s' " - "for argument '%s': %s", - p, - arg_name, - e, + error_message = ( + "unable to validate extra parameter '%s' for argument '%s': %s" + % (p, arg_name, e) ) - raise MoulinetteError("error_see_log") + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) return parameters @@ -478,10 +476,12 @@ class ActionsMap(object): try: mod = import_module("moulinette.authenticators.%s" % auth_conf["vendor"]) except ImportError: - logger.exception( - "unable to load authenticator vendor '%s'", auth_conf["vendor"] + error_message = ( + "unable to load authenticator vendor module 'moulinette.authenticators.%s'" + % auth_conf["vendor"] ) - raise MoulinetteError("error_see_log") + logger.exception(error_message) + raise MoulinetteError(error_message, raw_msg=True) else: return mod.Authenticator(**auth_conf) @@ -561,12 +561,17 @@ class ActionsMap(object): time() - start, ) func = getattr(mod, func_name) - except (AttributeError, ImportError): + except (AttributeError, ImportError) as e: import traceback traceback.print_exc() - logger.exception("unable to load function %s.%s", namespace, func_name) - raise MoulinetteError("error_see_log") + error_message = "unable to load function %s.%s because: %s" % ( + namespace, + func_name, + e, + ) + logger.exception(error_message) + raise MoulinetteError(error_message, raw_msg=True) else: log_id = start_action_logging() if logger.isEnabledFor(logging.DEBUG): diff --git a/moulinette/interfaces/__init__.py b/moulinette/interfaces/__init__.py index 03ee8261..e8c55c1b 100644 --- a/moulinette/interfaces/__init__.py +++ b/moulinette/interfaces/__init__.py @@ -250,24 +250,24 @@ class BaseActionsMapParser(object): # Store only if authentication is needed conf["authenticate"] = True if self.interface in ifaces else False else: - logger.error( + error_message = ( "expecting 'all', 'False' or a list for " - "configuration 'authenticate', got %r", - ifaces, + "configuration 'authenticate', got %r" % ifaces, ) - raise MoulinetteError("error_see_log") + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) # -- 'authenticator' auth = configuration.get("authenticator", "default") if not is_global and isinstance(auth, str): # Store needed authenticator profile if auth not in self.global_conf["authenticator"]: - logger.error( + error_message = ( "requesting profile '%s' which is undefined in " - "global configuration of 'authenticator'", - auth, + "global configuration of 'authenticator'" % auth, ) - raise MoulinetteError("error_see_log") + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) else: conf["authenticator"] = auth elif is_global and isinstance(auth, dict): @@ -286,12 +286,13 @@ class BaseActionsMapParser(object): } conf["authenticator"] = auths else: - logger.error( + error_message = ( "expecting a dict of profile(s) or a profile name " "for configuration 'authenticator', got %r", auth, ) - raise MoulinetteError("error_see_log") + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) return conf @@ -371,12 +372,13 @@ class _CallbackAction(argparse.Action): try: # Execute callback and get returned value value = self.callback(namespace, values, **self.callback_kwargs) - except Exception: - logger.exception( + except Exception as e: + error_message = ( "cannot get value from callback method " - "'{0}'".format(self.callback_method) + "'{0}': {1}".format(self.callback_method, e) ) - raise MoulinetteError("error_see_log") + logger.exception(error_message) + raise MoulinetteError(error_message, raw_msg=True) else: if value: if self.callback_return: diff --git a/moulinette/interfaces/api.py b/moulinette/interfaces/api.py index 033c3f72..b2d245ef 100644 --- a/moulinette/interfaces/api.py +++ b/moulinette/interfaces/api.py @@ -675,9 +675,13 @@ class ActionsMapParser(BaseActionsMapParser): try: # Retrieve the tid for the route tid, _ = self._parsers[kwargs.get("route")] - except KeyError: - logger.error("no argument parser found for route '%s'", kwargs.get("route")) - raise MoulinetteError("error_see_log") + except KeyError as e: + error_message = "no argument parser found for route '%s': %s" % ( + kwargs.get("route"), + e, + ) + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) if self.get_conf(tid, "authenticate"): authenticator = self.get_conf(tid, "authenticator") @@ -703,9 +707,10 @@ class ActionsMapParser(BaseActionsMapParser): try: # Retrieve the parser for the route _, parser = self._parsers[route] - except KeyError: - logger.error("no argument parser found for route '%s'", route) - raise MoulinetteError("error_see_log") + except KeyError as e: + error_message = "no argument parser found for route '%s': %s" % (route, e) + logger.error(error_message) + raise MoulinetteError(error_message, raw_msg=True) ret = argparse.Namespace() # TODO: Catch errors? @@ -825,10 +830,15 @@ class Interface(BaseInterface): server = WSGIServer((host, port), self._app, handler_class=WebSocketHandler) server.serve_forever() except IOError as e: - logger.exception("unable to start the server instance on %s:%d", host, port) + error_message = "unable to start the server instance on %s:%d: %s" % ( + host, + port, + e, + ) + logger.exception(error_message) if e.args[0] == errno.EADDRINUSE: raise MoulinetteError("server_already_running") - raise MoulinetteError("error_see_log") + raise MoulinetteError(error_message) # Routes handlers diff --git a/moulinette/interfaces/cli.py b/moulinette/interfaces/cli.py index 28321e5c..250e9379 100644 --- a/moulinette/interfaces/cli.py +++ b/moulinette/interfaces/cli.py @@ -380,9 +380,13 @@ class ActionsMapParser(BaseActionsMapParser): ret = self._parser.parse_args(args) except SystemExit: raise - except Exception: - logger.exception("unable to parse arguments '%s'", " ".join(args)) - raise MoulinetteError("error_see_log") + except Exception as e: + error_message = "unable to parse arguments '%s' because: %s" % ( + " ".join(args), + e, + ) + logger.exception(error_message) + raise MoulinetteError(error_message, raw_msg=True) tid = getattr(ret, "_tid", None) if self.get_conf(tid, "authenticate"): @@ -404,9 +408,13 @@ class ActionsMapParser(BaseActionsMapParser): ret = self._parser.parse_args(args) except SystemExit: raise - except Exception: - logger.exception("unable to parse arguments '%s'", " ".join(args)) - raise MoulinetteError("error_see_log") + except Exception as e: + error_message = "unable to parse arguments '%s' because: %s" % ( + " ".join(args), + e, + ) + logger.exception(error_message) + raise MoulinetteError(error_message, raw_msg=True) else: self.prepare_action_namespace(getattr(ret, "_tid", None), ret) self._parser.dequeue_callbacks(ret) diff --git a/test/conftest.py b/test/conftest.py index 793a0703..9008924b 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -27,7 +27,7 @@ def patch_translate(moulinette): def new_translate(self, key, *args, **kwargs): if key not in self._translations[self.default_locale].keys(): - message = "Unable to retrieve key %s for default locale!" % key + message = "Unable to retrieve key '%s' for default locale!" % key raise KeyError(message) return old_translate(self, key, *args, **kwargs) diff --git a/test/test_actionsmap.py b/test/test_actionsmap.py index 9055b89f..7614520d 100644 --- a/test/test_actionsmap.py +++ b/test/test_actionsmap.py @@ -188,7 +188,7 @@ def test_extra_argument_parser_add_argument_bad_arg(iface): with pytest.raises(MoulinetteError) as exception: extra_argument_parse.add_argument(GLOBAL_SECTION, "foo", {"ask": 1}) - translation = m18n.g("error_see_log") + translation = m18n.g("error") expected_msg = translation.format() assert expected_msg in str(exception) @@ -265,7 +265,7 @@ def test_actions_map_import_error(mocker): with pytest.raises(MoulinetteError) as exception: amap.process({}, timeout=30, route=("GET", "/test-auth/none")) - translation = m18n.g("error_see_log") + translation = m18n.g("error") expected_msg = translation.format() assert expected_msg in str(exception)