Merge pull request #1463 from lldelisle/lint_workflow

Increase worflow linting

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"])

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

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,

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,

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

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

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}.")