From 1b0d417076bcf4bf9d9dd25679de182656d2873c Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 1 May 2015 12:43:11 -0400 Subject: [PATCH] Exception handling improvements. For shed operations - catch exceptions, log them, but continue to completion and then return a 254 exit code at the end indicating there were problems with one or more repositories. This behavior has can be disabled with any of the shed operations using the new --fail_fast option. Clean up noisy exception reported by @peterjc @ https://github.com/galaxyproject/planemo/issues/158#issuecomment-98151153 (unless --fail_fast is used - then you get the full stack trace). Tests. Fixes #114. --- planemo/commands/cmd_shed_diff.py | 1 + planemo/commands/cmd_shed_download.py | 1 + planemo/commands/cmd_shed_lint.py | 1 + planemo/commands/cmd_shed_upload.py | 1 + planemo/options.py | 10 ++++ planemo/shed/__init__.py | 68 +++++++++++++++++++++------ planemo/shed_lint.py | 1 + tests/test_shed_lint.py | 25 +++++++++- tests/test_shed_upload.py | 2 +- 9 files changed, 93 insertions(+), 17 deletions(-) diff --git a/planemo/commands/cmd_shed_diff.py b/planemo/commands/cmd_shed_diff.py index 60f850ee8..25467ff12 100644 --- a/planemo/commands/cmd_shed_diff.py +++ b/planemo/commands/cmd_shed_diff.py @@ -16,6 +16,7 @@ @options.shed_owner_option() @options.shed_name_option() @options.shed_target_option() +@options.shed_fail_fast_option() @click.option( "-o", "--output", type=click.Path(file_okay=True, resolve_path=True), diff --git a/planemo/commands/cmd_shed_download.py b/planemo/commands/cmd_shed_download.py index 6dae93a01..efd5a4f43 100644 --- a/planemo/commands/cmd_shed_download.py +++ b/planemo/commands/cmd_shed_download.py @@ -32,6 +32,7 @@ @options.shed_name_option() @options.shed_target_option() @options.recursive_shed_option() +@options.shed_fail_fast_option() @pass_context def cli(ctx, path, **kwds): """Download a tool repository as a tarball from the tool shed and extract diff --git a/planemo/commands/cmd_shed_lint.py b/planemo/commands/cmd_shed_lint.py index 18cba2c4a..112368347 100644 --- a/planemo/commands/cmd_shed_lint.py +++ b/planemo/commands/cmd_shed_lint.py @@ -19,6 +19,7 @@ ) @options.lint_xsd_option() @options.recursive_shed_option() +@options.shed_fail_fast_option() @pass_context def cli(ctx, path, **kwds): """Check a Tool Shed repository for common problems. diff --git a/planemo/commands/cmd_shed_upload.py b/planemo/commands/cmd_shed_upload.py index 2b31e9d34..9b7412740 100644 --- a/planemo/commands/cmd_shed_upload.py +++ b/planemo/commands/cmd_shed_upload.py @@ -49,6 +49,7 @@ default=False ) @options.recursive_shed_option() +@options.shed_fail_fast_option() @pass_context def cli(ctx, path, **kwds): """Handle possible recursion through paths for uploading files to a toolshed diff --git a/planemo/options.py b/planemo/options.py index cc56d09d2..79e06e8ff 100644 --- a/planemo/options.py +++ b/planemo/options.py @@ -272,6 +272,16 @@ def shed_password_option(): ) +def shed_fail_fast_option(): + return click.option( + '--fail_fast', + is_flag=True, + default=False, + help="If multiple repositories are specified and an error occurs " + "stop immediately instead of processing remaining repositories." + ) + + def lint_xsd_option(): return click.option( '--xsd', diff --git a/planemo/shed/__init__.py b/planemo/shed/__init__.py index 0b07630f2..db22e9126 100644 --- a/planemo/shed/__init__.py +++ b/planemo/shed/__init__.py @@ -53,6 +53,8 @@ "repositories in .shed.yml at this time.") AUTO_NAME_CONFLICT_MESSAGE = ("Cannot specify both auto_tool_repositories and " "in .shed.yml and --name on the command-line.") +REALIZAION_PROBLEMS_MESSAGE = ("Problem encountered executing action for one or more " + "repositories.") # Planemo generated or consumed files that do not need to be uploaded to the # tool shed. PLANEMO_FILES = [ @@ -487,10 +489,15 @@ def build_tarball(realized_path, **kwds): def for_each_repository(function, path, **kwds): ret_codes = [] effective_repositories = realize_effective_repositories(path, **kwds) - for realized_repository in effective_repositories: - ret_codes.append( - function(realized_repository) - ) + try: + for realized_repository in effective_repositories: + ret_codes.append( + function(realized_repository) + ) + except RealizationException: + error(REALIZAION_PROBLEMS_MESSAGE) + return 254 + # "Good" returns are Nones, everything else is a -1 and should be # passed upwards. return 0 if all((not x) for x in ret_codes) else -1 @@ -528,16 +535,29 @@ def realize_effective_repositories(path, **kwds): tool). """ raw_repo_objects = _find_raw_repositories(path, **kwds) + failed = False temp_directory = mkdtemp() try: for raw_repo_object in raw_repo_objects: - for realized_repo in raw_repo_object.realizations( + if isinstance(raw_repo_object, Exception): + _handle_realization_error(raw_repo_object, **kwds) + failed = True + continue + + realized_repos = raw_repo_object.realizations( temp_directory, kwds.get("fail_on_missing", True) - ): + ) + for realized_repo in realized_repos: + if isinstance(realized_repo, Exception): + _handle_realization_error(realized_repo, **kwds) + failed = True + continue yield realized_repo finally: shutil.rmtree(temp_directory) + if failed: + raise RealizationException() def _create_shed_config(ctx, path, **kwds): @@ -615,7 +635,12 @@ def _find_raw_repositories(path, **kwds): config_name = None if len(shed_file_dirs) == 1: - config = shed_repo_config(shed_file_dirs[0], name=name) + shed_file_dir = shed_file_dirs[0] + try: + config = shed_repo_config(shed_file_dir, name=name) + except Exception as e: + error_message = PARSING_PROBLEM % (shed_file_dir, e) + return [RuntimeError(error_message)] config_name = config.get("name", None) if len(shed_file_dirs) > 1 and name is not None: @@ -637,19 +662,18 @@ def _build_raw_repo_objects(raw_dirs, **kwds): """ multiple = len(raw_dirs) > 1 name = kwds.get("name", None) - skip_errors = kwds.get("skip_errors", False) + # List of RawRepositoryDirectories or parsing failures if + # fail_fast is not enabled. raw_repo_objects = [] for raw_dir in raw_dirs: try: config = shed_repo_config(raw_dir, name=name) except Exception as e: - if skip_errors: - error_message = PARSING_PROBLEM % (raw_dir, e) - error(error_message) - continue - else: - raise + error_message = PARSING_PROBLEM % (raw_dir, e) + exception = RuntimeError(error_message) + _handle_realization_error(exception, **kwds) + raw_repo_objects.append(exception) raw_repo_object = RawRepositoryDirectory(raw_dir, config, multiple) raw_repo_objects.append(raw_repo_object) return raw_repo_objects @@ -709,7 +733,7 @@ def _realize_to(self, directory, name, multiple, fail_on_missing): missing = realized_files.include_failures if missing and fail_on_missing: msg = "Failed to include files for %s" % missing - raise Exception(msg) + return RuntimeError(msg) for realized_file in realized_files.files: relative_dest = realized_file.relative_dest @@ -918,6 +942,14 @@ def _shed_config_excludes(config): return config.get('ignore', []) + config.get('exclude', []) +def _handle_realization_error(exception, **kwds): + fail_fast = kwds.get("fail_fast", False) + if fail_fast: + raise exception + else: + error(str(exception)) + + def validate_repo_name(name): def _build_error(descript): return "Repository name [%s] invalid. %s" % (name, descript) @@ -956,6 +988,12 @@ def _build_error(descript): return msg +class RealizationException(Exception): + """ This exception indicates there was a problem while + realizing effective repositories for a shed command. As a + precondition - the user has already been informed with error(). + """ + __all__ = [ 'for_each_repository', 'api_exception_to_message', diff --git a/planemo/shed_lint.py b/planemo/shed_lint.py index ef8b7fdb4..d0272e877 100644 --- a/planemo/shed_lint.py +++ b/planemo/shed_lint.py @@ -152,6 +152,7 @@ def lint_shed_yaml(path, lint_ctx): shed_contents = yaml.load(open(shed_yaml, "r")) except Exception as e: lint_ctx.warn("Failed to parse .shed.yml file [%s]" % str(e)) + return lint_ctx.info(".shed.yml found and appears to be valid YAML.") _lint_shed_contents(lint_ctx, path, shed_contents) diff --git a/tests/test_shed_lint.py b/tests/test_shed_lint.py index f7d6db7f5..2d24833ac 100644 --- a/tests/test_shed_lint.py +++ b/tests/test_shed_lint.py @@ -1,4 +1,4 @@ - +from os.path import join from .test_utils import CliTestCase @@ -26,3 +26,26 @@ def test_invalid_repos(self): self._check_exit_code(["shed_lint"], exit_code=-1) with self._isolate_repo("bad_missing_include"): self._check_exit_code(["shed_lint"], exit_code=-1) + with self._isolate_repo("bad_invalid_yaml"): + self._check_exit_code(["shed_lint"], exit_code=254) + + def test_invalid_nested(self): + # Created a nested repository with one good and one + # invalid repository and make sure it runs and produces + # a 254 (it ran to completion but one or more things failed + # ) + with self._isolate() as f: + for name in ["bad_invalid_yaml", "single_tool_exclude"]: + self._copy_repo(name, join(f, name)) + self._copy_repo(name, join(f, name)) + self._check_exit_code(["shed_lint", "-r"], exit_code=254) + + def test_fail_fast(self): + # Created a nested repository with one good and one + # invalid repository and make sure it runs and produces + # a 254 (it ran to completion but one or more things failed + # ) + with self._isolate_repo("bad_invalid_yaml"): + r = self._check_exit_code(["shed_lint", "--fail_fast"], + exit_code=-1) + assert isinstance(r.exception, RuntimeError) diff --git a/tests/test_shed_upload.py b/tests/test_shed_upload.py index 8a837751b..06f46915f 100644 --- a/tests/test_shed_upload.py +++ b/tests/test_shed_upload.py @@ -52,7 +52,7 @@ def test_cannot_upload_missing_include(self): with self._isolate_repo("bad_missing_include"): upload_command = ["shed_upload", "--tar_only"] upload_command.extend(self._shed_args()) - self._check_exit_code(upload_command, exit_code=-1) + self._check_exit_code(upload_command, exit_code=254) def test_upload_recusrive(self): with self._isolate_repo("multi_repos_nested") as f: