diff --git a/planemo/commands/cmd_lint.py b/planemo/commands/cmd_lint.py index 85a3c77cc..f6ca5a454 100644 --- a/planemo/commands/cmd_lint.py +++ b/planemo/commands/cmd_lint.py @@ -3,7 +3,10 @@ import click from planemo import options -from planemo.cli import command_function +from planemo.cli import ( + command_function, + PlanemoCliContext, +) from planemo.tool_lint import ( build_tool_lint_args, lint_tools_on_path, @@ -43,7 +46,7 @@ # default=False, # ) @command_function -def cli(ctx, uris, **kwds): +def cli(ctx: PlanemoCliContext, uris, **kwds): """Check for common errors and best practices.""" lint_args = build_tool_lint_args(ctx, **kwds) exit_code = lint_tools_on_path(ctx, uris, lint_args, recursive=kwds["recursive"]) diff --git a/planemo/commands/cmd_shed_lint.py b/planemo/commands/cmd_shed_lint.py index 80f420b31..70bb5c9d4 100644 --- a/planemo/commands/cmd_shed_lint.py +++ b/planemo/commands/cmd_shed_lint.py @@ -7,7 +7,10 @@ shed, shed_lint, ) -from planemo.cli import command_function +from planemo.cli import ( + command_function, + PlanemoCliContext, +) @click.command("shed_lint") @@ -41,7 +44,7 @@ # default=False, # ) @command_function -def cli(ctx, paths, **kwds): +def cli(ctx: PlanemoCliContext, paths, **kwds): """Check Tool Shed repository for common issues. With the ``--tools`` flag, this command lints actual Galaxy tools diff --git a/planemo/commands/cmd_workflow_lint.py b/planemo/commands/cmd_workflow_lint.py index 80085c46d..e71fdd61f 100644 --- a/planemo/commands/cmd_workflow_lint.py +++ b/planemo/commands/cmd_workflow_lint.py @@ -3,9 +3,14 @@ import click from planemo import options -from planemo.cli import command_function -from planemo.lint import build_lint_args -from planemo.workflow_lint import lint_workflow_artifacts_on_paths +from planemo.cli import ( + command_function, + PlanemoCliContext, +) +from planemo.workflow_lint import ( + build_wf_lint_args, + lint_workflow_artifacts_on_paths, +) @click.command("workflow_lint") @@ -14,11 +19,17 @@ @options.report_xunit() @options.fail_level_option() @options.skip_option() +@click.option( + "--iwc", + is_flag=True, + default=False, + help="Check workflows directory with the standards of iwc", +) @command_function -def cli(ctx, paths, **kwds): +def cli(ctx: PlanemoCliContext, paths, **kwds): """Check workflows for syntax errors and best practices.""" # Unlike tools, lets just make this recursive by default. - lint_args = build_lint_args(ctx, **kwds) + lint_args = build_wf_lint_args(ctx, **kwds) exit_code = lint_workflow_artifacts_on_paths( ctx, paths, diff --git a/planemo/lint.py b/planemo/lint.py index c14247e7a..87bfd5aaa 100644 --- a/planemo/lint.py +++ b/planemo/lint.py @@ -1,6 +1,11 @@ """Utilities to help linting various targets.""" import os +from typing import ( + Any, + Dict, + TYPE_CHECKING, +) from urllib.request import urlopen import requests @@ -13,8 +18,11 @@ from planemo.shed import find_urls_for_xml from planemo.xml import validation +if TYPE_CHECKING: + from planemo.cli import PlanemoCliContext + -def build_lint_args(ctx, **kwds): +def build_lint_args(ctx: "PlanemoCliContext", **kwds) -> Dict[str, Any]: """Handle common report, error, and skip linting arguments.""" report_level = kwds.get("report_level", "all") fail_level = kwds.get("fail_level", "warn") @@ -39,7 +47,7 @@ def build_lint_args(ctx, **kwds): if len(invalid_skip_types): error(f"Unknown linter type(s) {invalid_skip_types} in list of linters to be skipped. Known linters {linters}") - lint_args = dict( + lint_args: Dict[str, Any] = dict( level=report_level, fail_level=fail_level, skip_types=skip_types, diff --git a/planemo/shed_lint.py b/planemo/shed_lint.py index 322d16756..b3d03760b 100644 --- a/planemo/shed_lint.py +++ b/planemo/shed_lint.py @@ -2,6 +2,7 @@ import os import xml.etree.ElementTree as ET +from typing import TYPE_CHECKING import yaml from galaxy.tool_util.lint import lint_tool_source_with @@ -31,6 +32,9 @@ from planemo.tools import yield_tool_sources from planemo.xml import XSDS_PATH +if TYPE_CHECKING: + from planemo.cli import PlanemoCliContext + TOOL_DEPENDENCIES_XSD = os.path.join(XSDS_PATH, "tool_dependencies.xsd") REPO_DEPENDENCIES_XSD = os.path.join(XSDS_PATH, "repository_dependencies.xsd") @@ -50,7 +54,7 @@ ] -def lint_repository(ctx, realized_repository, **kwds): +def lint_repository(ctx: "PlanemoCliContext", realized_repository, **kwds): """Lint a realized shed repository. See :mod:`planemo.shed` for details on constructing a realized diff --git a/planemo/tool_lint.py b/planemo/tool_lint.py index 9e72d2646..eea2ad7ff 100644 --- a/planemo/tool_lint.py +++ b/planemo/tool_lint.py @@ -1,4 +1,9 @@ from os.path import basename +from typing import ( + Any, + Dict, + TYPE_CHECKING, +) from galaxy.tool_util.lint import lint_tool_source @@ -22,10 +27,13 @@ yield_tool_sources_on_paths, ) +if TYPE_CHECKING: + from planemo.cli import PlanemoCliContext + LINTING_TOOL_MESSAGE = "Linting tool %s" -def build_tool_lint_args(ctx, **kwds): +def build_tool_lint_args(ctx: "PlanemoCliContext", **kwds) -> Dict[str, Any]: lint_args = build_lint_args(ctx, **kwds) extra_modules = _lint_extra_modules(**kwds) lint_args["extra_modules"] = extra_modules diff --git a/planemo/workflow_lint.py b/planemo/workflow_lint.py index fdc2bbc80..b8403f41c 100644 --- a/planemo/workflow_lint.py +++ b/planemo/workflow_lint.py @@ -12,7 +12,6 @@ Optional, Tuple, TYPE_CHECKING, - Union, ) import requests @@ -39,6 +38,7 @@ output_labels, required_input_labels, ) +from planemo.lint import build_lint_args from planemo.runnable import ( cases, for_path, @@ -59,6 +59,12 @@ class WorkflowLintContext(LintContext): training_topic = None +def build_wf_lint_args(ctx: "PlanemoCliContext", **kwds) -> Dict[str, Any]: + lint_args = build_lint_args(ctx, **kwds) + lint_args["iwc_grade"] = kwds.get("iwc", False) + return lint_args + + def generate_dockstore_yaml(directory: str, publish: bool = True) -> str: workflows = [] all_workflow_paths = list(find_workflow_descriptions(directory)) @@ -121,9 +127,7 @@ def generate_dockstore_yaml(directory: str, publish: bool = True) -> str: return contents -def lint_workflow_artifacts_on_paths( - ctx: "PlanemoCliContext", paths: Iterable[str], lint_args: Dict[str, Union[str, List[str]]] -) -> int: +def lint_workflow_artifacts_on_paths(ctx: "PlanemoCliContext", paths: Iterable[str], lint_args: Dict[str, Any]) -> int: report_level = lint_args["level"] lint_context = WorkflowLintContext(report_level, skip_types=lint_args["skip_types"]) for path in paths: @@ -135,12 +139,22 @@ def lint_workflow_artifacts_on_paths( return EXIT_CODE_OK -def _lint_workflow_artifacts_on_path( - lint_context: WorkflowLintContext, path: str, lint_args: Dict[str, Union[str, List[str]]] -) -> None: +def _lint_workflow_artifacts_on_path(lint_context: WorkflowLintContext, path: str, lint_args: Dict[str, Any]) -> None: + if lint_args["iwc_grade"]: + if not os.path.isdir(path): + path = os.path.dirname(path) + lint_context.lint("lint_required_files", _lint_required_files_workflow_dir, path) + lint_context.lint("lint_changelog", _lint_changelog_version, path) + for potential_workflow_artifact_path in find_potential_workflow_files(path): if os.path.basename(potential_workflow_artifact_path) == DOCKSTORE_REGISTRY_CONF: lint_context.lint("lint_dockstore", _lint_dockstore_config, potential_workflow_artifact_path) + if lint_args["iwc_grade"]: + lint_context.lint( + "lint_dockstore_best_practices", + _lint_dockstore_config_best_practices, + potential_workflow_artifact_path, + ) elif looks_like_a_workflow(potential_workflow_artifact_path): @@ -152,6 +166,8 @@ def structure(path, lint_context): lint_func(lint_context, workflow_dict, path=path) lint_context.lint("lint_structure", structure, potential_workflow_artifact_path) + if lint_args["iwc_grade"]: + lint_context.lint("lint_release", _lint_release, potential_workflow_artifact_path) lint_context.lint("lint_best_practices", _lint_best_practices, potential_workflow_artifact_path) lint_context.lint("lint_tests", _lint_tsts, potential_workflow_artifact_path) lint_context.lint("lint_tool_ids", _lint_tool_ids, potential_workflow_artifact_path) @@ -394,6 +410,11 @@ def _lint_dockstore_config(path: str, lint_context: WorkflowLintContext) -> None lint_context.error("Invalid YAML contents found in %s" % DOCKSTORE_REGISTRY_CONF) return + if "version" not in dockstore_yaml: + lint_context.error("Invalid YAML contents found in %s, no version defined" % DOCKSTORE_REGISTRY_CONF) + if str(dockstore_yaml.get("version")) != DOCKSTORE_REGISTRY_CONF_VERSION: + lint_context.error("Invalid YAML version found in %s." % DOCKSTORE_REGISTRY_CONF) + if "workflows" not in dockstore_yaml: lint_context.error("Invalid YAML contents found in %s, no workflows defined" % DOCKSTORE_REGISTRY_CONF) return @@ -403,8 +424,16 @@ def _lint_dockstore_config(path: str, lint_context: WorkflowLintContext) -> None lint_context.error("Invalid YAML contents found in %s, workflows not a list" % DOCKSTORE_REGISTRY_CONF) return + if len(workflow_entries) == 0: + lint_context.error("No workflow specified in the .dockstore.yml.") + + workflow_names_in_dockstore = [] for workflow_entry in workflow_entries: _lint_dockstore_workflow_entry(lint_context, os.path.dirname(path), workflow_entry) + workflow_name = workflow_entry.get("name", "") + if workflow_name in workflow_names_in_dockstore: + lint_context.error(f"{DOCKSTORE_REGISTRY_CONF} has multiple workflow entries with the same name") + workflow_names_in_dockstore.append(workflow_name) def _lint_dockstore_workflow_entry( @@ -420,15 +449,13 @@ def _lint_dockstore_workflow_entry( lint_context.error(f"{DOCKSTORE_REGISTRY_CONF} workflow entry missing required key {required_key}") found_errors = True - for recommended_key in ["testParameterFiles"]: - if recommended_key not in workflow_entry: - lint_context.warn(f"{DOCKSTORE_REGISTRY_CONF} workflow entry missing recommended key {recommended_key}") - if found_errors: # Don't do the rest of the validation for a broken file. return - # TODO: validate subclass + if workflow_entry.get("subclass") != "Galaxy": + lint_context.error(f"{DOCKSTORE_REGISTRY_CONF} workflow entry subclass must be 'Galaxy'.") + descriptor_path = workflow_entry["primaryDescriptorPath"] test_files = workflow_entry.get("testParameterFiles", []) @@ -437,6 +464,20 @@ def _lint_dockstore_workflow_entry( if not os.path.exists(referenced_path): lint_context.error(f"{DOCKSTORE_REGISTRY_CONF} workflow entry references absent file {referenced_file}") + # Check there is no space in name: + workflow_name = workflow_entry.get("name", "") + # Check the name has no space + if " " in workflow_name: + lint_context.error( + "Dockstore does not accept workflow names with space.", + f"Change '{workflow_name}' in {DOCKSTORE_REGISTRY_CONF}.", + ) + + # Check there is not mailto + for author in workflow_entry.get("authors", []): + if author.get("email", "").startswith("mailto:"): + lint_context.error("email field of the .dockstore.yml must not " "contain 'mailto:'") + def looks_like_a_workflow(path: str) -> bool: """Return boolean indicating if this path looks like a workflow.""" @@ -537,3 +578,80 @@ def _lint_tool_ids_steps(lint_context: WorkflowLintContext, wf_dict: Dict, ts: T if not failed: lint_context.valid("All tool ids appear to be valid.") return None + + +def _lint_required_files_workflow_dir(path: str, lint_context: WorkflowLintContext) -> None: + # Check all required files are present + required_files = ["README.md", "CHANGELOG.md", ".dockstore.yml"] + for required_file in required_files: + if not os.path.exists(os.path.join(path, required_file)): + lint_context.error(f"The file {required_file} is" " missing but required.") + + +def _get_changelog_version(path: str) -> str: + # Get the version from the CHANGELOG.md + version = "" + if not os.path.exists(os.path.join(path, "CHANGELOG.md")): + return version + with open(os.path.join(path, "CHANGELOG.md"), "r") as f: + for line in f: + if line.startswith("## ["): + version = line.split("]")[0].replace("## [", "") + break + return version + + +def _lint_changelog_version(path: str, lint_context: WorkflowLintContext) -> None: + # Check the version can be get from the CHANGELOG.md + if not os.path.exists(os.path.join(path, "CHANGELOG.md")): + return + if _get_changelog_version(path) == "": + lint_context.error( + "No version found in CHANGELOG. " + "The version should be in a line that starts like " + "'## [version number]'" + ) + + +def _lint_release(path, lint_context): + with open(path) as f: + workflow_dict = ordered_load(f) + if "release" not in workflow_dict: + lint_context.error(f"The workflow {path} has no release") + else: + # Try to get the version from the CHANGELOG: + version = _get_changelog_version(os.path.dirname(path)) + if version != "" and workflow_dict.get("release") != version: + lint_context.error(f"The release of workflow {path} does not match " "the version in the CHANGELOG.") + + +def _lint_dockstore_config_best_practices(path: str, lint_context: WorkflowLintContext) -> None: + dockstore_yaml = None + try: + with open(path) as f: + dockstore_yaml = yaml.safe_load(f) + except Exception: + return + + if not isinstance(dockstore_yaml, dict): + return + + workflow_entries = dockstore_yaml.get("workflows") + if not isinstance(workflow_entries, list): + return + + for workflow_entry in workflow_entries: + _lint_dockstore_workflow_entry_best_practices(lint_context, os.path.dirname(path), workflow_entry) + + +def _lint_dockstore_workflow_entry_best_practices( + lint_context: WorkflowLintContext, directory: str, workflow_entry: Dict[str, Any] +) -> None: + for recommended_key in ["testParameterFiles", "name"]: + if recommended_key not in workflow_entry: + lint_context.error(f"{DOCKSTORE_REGISTRY_CONF} workflow entry missing recommended key {recommended_key}") + + workflow_name = workflow_entry.get("name", "") + # Check there is at least one author + if len(workflow_entry.get("authors", [])) == 0: + lint_context.error(f"Workflow {workflow_name} have no 'authors' in the {DOCKSTORE_REGISTRY_CONF}.") diff --git a/tests/data/wf_repos/basic_wf_iwc_invalid_version/.dockstore.yml b/tests/data/wf_repos/basic_wf_iwc_invalid_version/.dockstore.yml new file mode 100644 index 000000000..50c716fee --- /dev/null +++ b/tests/data/wf_repos/basic_wf_iwc_invalid_version/.dockstore.yml @@ -0,0 +1,11 @@ +version: 1.2 +workflows: +- name: main + subclass: Galaxy + publish: true + primaryDescriptorPath: /Super-simple-workflow.ga + testParameterFiles: + - /Super-simple-workflow-tests.yml + authors: + - name: Lucille Delisle + orcid: 0000-0002-1964-4960 diff --git a/tests/data/wf_repos/basic_wf_iwc_invalid_version/CHANGELOG.md b/tests/data/wf_repos/basic_wf_iwc_invalid_version/CHANGELOG.md new file mode 100644 index 000000000..7a859c1bf --- /dev/null +++ b/tests/data/wf_repos/basic_wf_iwc_invalid_version/CHANGELOG.md @@ -0,0 +1,5 @@ +# Changelog + +## [0.1] 2024-06-17 + +First release. diff --git a/tests/data/wf_repos/basic_wf_iwc_invalid_version/README.md b/tests/data/wf_repos/basic_wf_iwc_invalid_version/README.md new file mode 100644 index 000000000..a26db8ed6 --- /dev/null +++ b/tests/data/wf_repos/basic_wf_iwc_invalid_version/README.md @@ -0,0 +1,3 @@ +# Super simple workflow + +This is a super simple workflow which generates a file with x lines with 'hello' diff --git a/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow-tests.yml b/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow-tests.yml new file mode 100644 index 000000000..9b1faff97 --- /dev/null +++ b/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow-tests.yml @@ -0,0 +1,10 @@ +- doc: Test outline for Super-simple-workflow + job: + n_rows: 5 + outputs: + outfile: + asserts: + has_n_lines: + n: 5 + has_line: + line: "hello" diff --git a/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow.ga b/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow.ga new file mode 100644 index 000000000..fcc756bd6 --- /dev/null +++ b/tests/data/wf_repos/basic_wf_iwc_invalid_version/Super-simple-workflow.ga @@ -0,0 +1,95 @@ +{ + "a_galaxy_workflow": "true", + "annotation": "This workflow generates a file with x lines with 'hello'", + "comments": [], + "creator": [ + { + "class": "Person", + "identifier": "https://orcid.org/0000-0002-1964-4960", + "name": "Lucille Delisle" + } + ], + "format-version": "0.1", + "license": "MIT", + "release": "0.2", + "name": "Super simple workflow", + "report": { + "markdown": "\n# Workflow Execution Report\n\n## Workflow Inputs\n```galaxy\ninvocation_inputs()\n```\n\n## Workflow Outputs\n```galaxy\ninvocation_outputs()\n```\n\n## Workflow\n```galaxy\nworkflow_display()\n```\n" + }, + "steps": { + "0": { + "annotation": "Number of rows to generate", + "content_id": null, + "errors": null, + "id": 0, + "input_connections": {}, + "inputs": [ + { + "description": "Number of rows to generate", + "name": "n_rows" + } + ], + "label": "n_rows", + "name": "Input parameter", + "outputs": [], + "position": { + "left": 0, + "top": 0 + }, + "tool_id": null, + "tool_state": "{\"parameter_type\": \"integer\", \"optional\": false}", + "tool_version": null, + "type": "parameter_input", + "uuid": "2ee42c13-83f5-4ac4-b35d-74294edf7dea", + "when": null + }, + "1": { + "annotation": "this creates a file with a given number of lines", + "content_id": "toolshed.g2.bx.psu.edu/repos/bgruening/text_processing/tp_text_file_with_recurring_lines/9.3+galaxy1", + "errors": null, + "id": 1, + "input_connections": { + "token_set_0|repeat_select|times": { + "id": 0, + "output_name": "output" + } + }, + "inputs": [], + "label": "create file", + "name": "Create text file", + "outputs": [ + { + "name": "outfile", + "type": "txt" + } + ], + "position": { + "left": 236, + "top": 11 + }, + "post_job_actions": {}, + "tool_id": "toolshed.g2.bx.psu.edu/repos/bgruening/text_processing/tp_text_file_with_recurring_lines/9.3+galaxy1", + "tool_shed_repository": { + "changeset_revision": "fbf99087e067", + "name": "text_processing", + "owner": "bgruening", + "tool_shed": "toolshed.g2.bx.psu.edu" + }, + "tool_state": "{\"token_set\": [{\"__index__\": 0, \"line\": \"hello\", \"repeat_select\": {\"repeat_select_opts\": \"user\", \"__current_case__\": 0, \"times\": {\"__class__\": \"ConnectedValue\"}}}], \"__page__\": null, \"__rerun_remap_job_id__\": null}", + "tool_version": "9.3+galaxy1", + "type": "tool", + "uuid": "a86ff433-8dce-417c-baf0-bc106a93ee48", + "when": null, + "workflow_outputs": [ + { + "label": "outfile", + "output_name": "outfile", + "uuid": "34572088-8ad8-4661-81a2-6dfe2d83a0fe" + } + ] + } + }, + "tags": [], + "uuid": "72c87042-27a8-4bcc-92af-ca74704e6161", + "version": 3 +} \ No newline at end of file diff --git a/tests/test_cmd_workflow_lint.py b/tests/test_cmd_workflow_lint.py index 02d38c2e7..9377464f5 100644 --- a/tests/test_cmd_workflow_lint.py +++ b/tests/test_cmd_workflow_lint.py @@ -226,6 +226,50 @@ def test_workflow_linting_asserts(self): result = self._runner.invoke(self._cli.planemo, lint_cmd) assert "ERROR: Invalid assertion in tests: assert_has_line missing a required argument: 'line'" in result.output + def test_workflow_linting_iwc(self): + # Check the output of workflow_lint --iwc on a basic workflow with .dockstore + for repo in [ + _wf_repo("basic_format2_dockstore"), + _wf_repo(os.path.join("basic_format2_dockstore", "basic_format2.gxwf.yml")), + ]: + lint_cmd = ["workflow_lint", "--skip", "best_practices", "--iwc", repo] + result = self._runner.invoke(self._cli.planemo, lint_cmd) + + errors = [ + "The file README.md is missing but required.", + "The file CHANGELOG.md is missing but required.", + ".dockstore.yml workflow entry missing recommended key name", + "Workflow have no 'authors' in the .dockstore.yml.", + "has no release", + ] + + for error in errors: + assert error in result.output + + # Check that skipping the good steps makes it work + lint_cmd = [ + "workflow_lint", + "--iwc", + "--skip", + "best_practices,required_files,dockstore_best_practices,release", + repo, + ] + self._check_exit_code(lint_cmd, exit_code=0) + + # Check the output of workflow_lint --iwc on a good workflow but with an issue with the release + repo = _wf_repo("basic_wf_iwc_invalid_version") + lint_cmd = ["workflow_lint", "--iwc", repo] + result = self._runner.invoke(self._cli.planemo, lint_cmd) + + errors = ["The release of workflow", " does not match the version in the CHANGELOG."] + + for error in errors: + assert error in result.output + + # Check that skipping the good steps makes it work + lint_cmd = ["workflow_lint", "--iwc", "--skip", "release", repo] + self._check_exit_code(lint_cmd, exit_code=0) + def _wf_repo(rel_path): return os.path.join(TEST_DATA_DIR, "wf_repos", rel_path)