diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f10292826..6c4265518e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index deb47b5799..d27797c7f2 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/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. diff --git a/tests/modules/lint.py b/tests/modules/lint.py index d31f2c3212..e26d3e3103 100644 --- a/tests/modules/lint.py +++ b/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)}." diff --git a/tests/test_modules.py b/tests/test_modules.py index 047369b7c3..d4c8b9fc52 100644 --- a/tests/test_modules.py +++ b/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,