Merge pull request #2489 from fa2k/check-spaces-in-container-links

Check spaces in container links

CHANGELOG.md

@@ -10,6 +10,7 @@
 
 - Fix incorrectly failing linting if 'modules' was not found in meta.yml ([#2447](https://github.com/nf-core/tools/pull/2447))
 - Correctly pass subworkflow linting test if `COMPONENT.out.versions` is used in the script ([#2448](https://github.com/nf-core/tools/pull/2448))
+- Check for spaces in modules container URLs ([#2452](https://github.com/nf-core/tools/issues/2452))
 
 ### Modules
 

nf_core/modules/lint/main_nf.py

@@ -246,9 +246,9 @@ def check_process_section(self, lines, registry, fix_version, progress_bar):
     check_process_labels(self, lines)
 
     # Deprecated enable_conda
-    for i, l in enumerate(lines):
+    for i, raw_line in enumerate(lines):
         url = None
-        l = l.strip(" \n'\"}:")
+        l = raw_line.strip(" \n'\"}:")
 
         # Catch preceeding "container "
         if l.startswith("container"):
@@ -314,34 +314,9 @@ def check_process_section(self, lines, registry, fix_version, progress_bar):
                 l = "/".join([registry, l]).replace("//", "/")
             url = urlparse(l.split("'")[0])
 
-        # lint double quotes
         if l.startswith("container") or _container_type(l) == "docker" or _container_type(l) == "singularity":
-            if l.count('"') > 2:
-                self.failed.append(
-                    (
-                        "container_links",
-                        f"Too many double quotes found when specifying container: {l.lstrip('container ')}",
-                        self.main_nf,
-                    )
-                )
-            else:
-                self.passed.append(
-                    (
-                        "container_links",
-                        f"Correct number of double quotes found when specifying container: {l.lstrip('container ')}",
-                        self.main_nf,
-                    )
-                )
+            check_container_link_line(self, raw_line, registry)
 
-        # lint more than one container in the same line
-        if ("https://containers" in l or "https://depot" in l) and ("biocontainers/" in l or l.startswith(registry)):
-            self.warned.append(
-                (
-                    "container_links",
-                    "Docker and Singularity containers specified in the same line. Only first one checked.",
-                    self.main_nf,
-                )
-            )
         # Try to connect to container URLs
         if url is None:
             continue
@@ -484,6 +459,68 @@ def check_process_labels(self, lines):
         self.warned.append(("process_standard_label", "Process label not specified", self.main_nf))
 
 
+def check_container_link_line(self, raw_line, registry):
+    """Look for common problems in the container name / URL, for docker and singularity."""
+
+    l = raw_line.strip(" \n'\"}:")
+
+    # lint double quotes
+    if l.count('"') > 2:
+        self.failed.append(
+            (
+                "container_links",
+                f"Too many double quotes found when specifying container: {l.lstrip('container ')}",
+                self.main_nf,
+            )
+        )
+    else:
+        self.passed.append(
+            (
+                "container_links",
+                f"Correct number of double quotes found when specifying container: {l.lstrip('container ')}",
+                self.main_nf,
+            )
+        )
+
+    # Check for spaces in url
+    single_quoted_items = raw_line.split("'")
+    double_quoted_items = raw_line.split('"')
+    # Look for container link as single item surrounded by quotes
+    # (if there are multiple links, this will be warned in the next check)
+    container_link = None
+    if len(single_quoted_items) == 3:
+        container_link = single_quoted_items[1]
+    elif len(double_quoted_items) == 3:
+        container_link = double_quoted_items[1]
+    if container_link:
+        if " " in container_link:
+            self.failed.append(
+                (
+                    "container_links",
+                    f"Space character found in container: '{container_link}'",
+                    self.main_nf,
+                )
+            )
+        else:
+            self.passed.append(
+                (
+                    "container_links",
+                    f"No space characters found in container: '{container_link}'",
+                    self.main_nf,
+                )
+            )
+
+        # lint more than one container in the same line
+        if ("https://containers" in l or "https://depot" in l) and ("biocontainers/" in l or l.startswith(registry)):
+            self.warned.append(
+                (
+                    "container_links",
+                    "Docker and Singularity containers specified in the same line. Only first one checked.",
+                    self.main_nf,
+                )
+            )
+
+
 def _parse_input(self, line_raw):
     """
     Return list of input channel names from an input line.

tests/modules/lint.py

@@ -231,3 +231,78 @@ def test_modules_lint_check_process_labels(self):
         assert len(mocked_ModuleLint.passed) == passed
         assert len(mocked_ModuleLint.warned) == warned
         assert len(mocked_ModuleLint.failed) == failed
+
+
+# Test cases for linting the container definitions
+
+CONTAINER_SINGLE_GOOD = (
+    "Single-line container definition should pass",
+    """
+    container "quay.io/nf-core/gatk:4.4.0.0" //Biocontainers is missing a package
+    """,
+    2,  # passed
+    0,  # warned
+    0,  # failed
+)
+
+CONTAINER_TWO_LINKS_GOOD = (
+    "Multi-line container definition should pass",
+    """
+    container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
+        'https://depot.galaxyproject.org/singularity/gatk4:4.4.0.0--py36hdfd78af_0':
+        'biocontainers/gatk4:4.4.0.0--py36hdfd78af_0' }"
+    """,
+    6,
+    0,
+    0,
+)
+
+CONTAINER_WITH_SPACE_BAD = (
+    "Space in container URL should fail",
+    """
+    container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
+        'https://depot.galaxyproject.org/singularity/gatk4:4.4.0.0--py36hdfd78af_0 ':
+        'biocontainers/gatk4:4.4.0.0--py36hdfd78af_0' }"
+    """,
+    5,
+    0,
+    1,
+)
+
+CONTAINER_MULTIPLE_DBLQUOTES_BAD = (
+    "Incorrect quoting of container string should fail",
+    """
+    container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
+        'https://depot.galaxyproject.org/singularity/gatk4:4.4.0.0--py36hdfd78af_0 ':
+        "biocontainers/gatk4:4.4.0.0--py36hdfd78af_0" }"
+    """,
+    4,
+    0,
+    1,
+)
+
+CONTAINER_TEST_CASES = [
+    CONTAINER_SINGLE_GOOD,
+    CONTAINER_TWO_LINKS_GOOD,
+    CONTAINER_WITH_SPACE_BAD,
+    CONTAINER_MULTIPLE_DBLQUOTES_BAD,
+]
+
+
+def test_modules_lint_check_url(self):
+    for test_case in CONTAINER_TEST_CASES:
+        test, process, passed, warned, failed = test_case
+        mocked_ModuleLint = MockModuleLint()
+        for line in process.splitlines():
+            if line.strip():
+                main_nf.check_container_link_line(mocked_ModuleLint, line, registry="quay.io")
+
+        assert (
+            len(mocked_ModuleLint.passed) == passed
+        ), f"{test}: Expected {passed} PASS, got {len(mocked_ModuleLint.passed)}."
+        assert (
+            len(mocked_ModuleLint.warned) == warned
+        ), f"{test}: Expected {warned} WARN, got {len(mocked_ModuleLint.warned)}."
+        assert (
+            len(mocked_ModuleLint.failed) == failed
+        ), f"{test}: Expected {failed} FAIL, got {len(mocked_ModuleLint.failed)}."

tests/test_modules.py

@@ -174,6 +174,7 @@ def test_modulesrepo_class(self):
     )
     from .modules.lint import (
         test_modules_lint_check_process_labels,
+        test_modules_lint_check_url,
         test_modules_lint_empty,
         test_modules_lint_gitlab_modules,
         test_modules_lint_multiple_remotes,