From 12b8e644135665ba0a732434818c2201d2fe681d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Bj=C3=B8rnstad?= Date: Wed, 18 Oct 2023 10:59:54 +0200 Subject: [PATCH 1/5] Check for spaces in container urls (WIP) --- CHANGELOG.md | 1 + nf_core/modules/lint/main_nf.py | 34 ++++++++++++-- tests/modules/lint.py | 81 +++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 645a7f436f..357facaa89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ ### Linting - Add new command `nf-core subworkflows lint` ([#2379](https://github.com/nf-core/tools/pull/2379)) +- Check for spaces in 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..05539f8031 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,8 +314,8 @@ 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": + # lint double quotes if l.count('"') > 2: self.failed.append( ( @@ -333,6 +333,34 @@ def check_process_section(self, lines, registry, fix_version, progress_bar): ) ) + # 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 container_link.contains(" "): + 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( diff --git a/tests/modules/lint.py b/tests/modules/lint.py index d31f2c3212..ef62248606 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -231,3 +231,84 @@ 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 = ( + """ + //Conda is not supported at the moment: https://github.com/broadinstitute/gatk/issues/7811 + container "quay.io/nf-core/gatk:4.4.0.0" //Biocontainers is missing a package + """, + 1, # passed + 0, # warned + 0, # failed +) + +CONTAINER_TWO_LINKS_GOOD = ( + """ + conda "bioconda::gatk4=4.4.0.0" + 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' }" + """, + 1, + 0, + 0, +) + +CONTAINER_WITH_SPACE_BAD = ( + """ + conda "bioconda::gatk4=4.4.0.0" + 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' }" + """, + 0, + 1, + 0, +) + +CONTAINER_WITH_SPACE_BAD = ( + """ + conda "bioconda::gatk4=4.4.0.0" + 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' }" + """, + 0, + 1, + 0, +) + +CONTAINER_MULTIPLE_DBLQUOTES_BAD = ( + """ + conda "bioconda::gatk4=4.4.0.0" + 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" }" + """, + 0, + 1, + 0, +) + +CONTAINER_TEST_CASES = [ + CONTAINER_SINGLE_GOOD, + CONTAINER_TWO_LINKS_GOOD, + CONTAINER_WITH_SPACE_BAD, + CONTAINER_WITH_SPACE_BAD, + CONTAINER_MULTIPLE_DBLQUOTES_BAD, +] + + +def test_modules_lint_check_process_labels(self): + for test_case in CONTAINER_TEST_CASES: + process, passed, warned, failed = test_case + mocked_ModuleLint = MockModuleLint() + main_nf.check_process_section( + mocked_ModuleLint, process.splitlines(), registry="quay.io", fix_version=False, progress_bar=False + ) + assert len(mocked_ModuleLint.passed) == passed + assert len(mocked_ModuleLint.warned) == warned + assert len(mocked_ModuleLint.failed) == failed From 4e6878f34e07ef58d2bad15b1b7336948bf874dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Bj=C3=B8rnstad?= Date: Mon, 23 Oct 2023 13:41:04 +0200 Subject: [PATCH 2/5] Fix code & remove duplicate test case --- nf_core/modules/lint/main_nf.py | 2 +- tests/modules/lint.py | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 05539f8031..7c9d8699c6 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -344,7 +344,7 @@ def check_process_section(self, lines, registry, fix_version, progress_bar): elif len(double_quoted_items) == 3: container_link = double_quoted_items[1] if container_link: - if container_link.contains(" "): + if ' ' in container_link: self.failed.append( ( "container_links", diff --git a/tests/modules/lint.py b/tests/modules/lint.py index ef62248606..4b737f542e 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -269,18 +269,6 @@ def test_modules_lint_check_process_labels(self): 0, ) -CONTAINER_WITH_SPACE_BAD = ( - """ - conda "bioconda::gatk4=4.4.0.0" - 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' }" - """, - 0, - 1, - 0, -) - CONTAINER_MULTIPLE_DBLQUOTES_BAD = ( """ conda "bioconda::gatk4=4.4.0.0" @@ -297,7 +285,6 @@ def test_modules_lint_check_process_labels(self): CONTAINER_SINGLE_GOOD, CONTAINER_TWO_LINKS_GOOD, CONTAINER_WITH_SPACE_BAD, - CONTAINER_WITH_SPACE_BAD, CONTAINER_MULTIPLE_DBLQUOTES_BAD, ] From 0367be588bfd3211ae35db62b4df14323c95a21b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Bj=C3=B8rnstad?= Date: Mon, 23 Oct 2023 13:47:54 +0200 Subject: [PATCH 3/5] Use doublequotes (Black) --- nf_core/modules/lint/main_nf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 7c9d8699c6..1fa57432ac 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -344,7 +344,7 @@ def check_process_section(self, lines, registry, fix_version, progress_bar): elif len(double_quoted_items) == 3: container_link = double_quoted_items[1] if container_link: - if ' ' in container_link: + if " " in container_link: self.failed.append( ( "container_links", From 3f3fc9f53363196aca7693f911e0e7fc3a999d27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Bj=C3=B8rnstad?= Date: Mon, 23 Oct 2023 15:57:47 +0200 Subject: [PATCH 4/5] Move to new release --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8035e27a8c..172a8a5d17 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 @@ -50,7 +51,6 @@ - Add new command `nf-core subworkflows lint` ([#2379](https://github.com/nf-core/tools/pull/2379)) - Check for existence of test profile ([#2478](https://github.com/nf-core/tools/pull/2478)) -- Check for spaces in container URLs ([#2452](https://github.com/nf-core/tools/issues/2452)) ### Modules From 3c6af2d22f08e6be834aa9b304c1b481d1383237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20Bj=C3=B8rnstad?= Date: Tue, 24 Oct 2023 13:18:34 +0200 Subject: [PATCH 5/5] Extracted container link check, fix tests --- nf_core/modules/lint/main_nf.py | 117 +++++++++++++++++--------------- tests/modules/lint.py | 39 ++++++----- tests/test_modules.py | 1 + 3 files changed, 87 insertions(+), 70 deletions(-) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 1fa57432ac..d27797c7f2 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -315,61 +315,8 @@ def check_process_section(self, lines, registry, fix_version, progress_bar): url = urlparse(l.split("'")[0]) if l.startswith("container") or _container_type(l) == "docker" or _container_type(l) == "singularity": - # 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_container_link_line(self, raw_line, registry) - # 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, - ) - ) # Try to connect to container URLs if url is None: continue @@ -512,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 4b737f542e..e26d3e3103 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -236,49 +236,49 @@ def test_modules_lint_check_process_labels(self): # Test cases for linting the container definitions CONTAINER_SINGLE_GOOD = ( + "Single-line container definition should pass", """ - //Conda is not supported at the moment: https://github.com/broadinstitute/gatk/issues/7811 container "quay.io/nf-core/gatk:4.4.0.0" //Biocontainers is missing a package """, - 1, # passed + 2, # passed 0, # warned 0, # failed ) CONTAINER_TWO_LINKS_GOOD = ( + "Multi-line container definition should pass", """ - conda "bioconda::gatk4=4.4.0.0" 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' }" """, - 1, + 6, 0, 0, ) CONTAINER_WITH_SPACE_BAD = ( + "Space in container URL should fail", """ - conda "bioconda::gatk4=4.4.0.0" 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, - 0, ) CONTAINER_MULTIPLE_DBLQUOTES_BAD = ( + "Incorrect quoting of container string should fail", """ - conda "bioconda::gatk4=4.4.0.0" 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, - 0, ) CONTAINER_TEST_CASES = [ @@ -289,13 +289,20 @@ def test_modules_lint_check_process_labels(self): ] -def test_modules_lint_check_process_labels(self): +def test_modules_lint_check_url(self): for test_case in CONTAINER_TEST_CASES: - process, passed, warned, failed = test_case + test, process, passed, warned, failed = test_case mocked_ModuleLint = MockModuleLint() - main_nf.check_process_section( - mocked_ModuleLint, process.splitlines(), registry="quay.io", fix_version=False, progress_bar=False - ) - assert len(mocked_ModuleLint.passed) == passed - assert len(mocked_ModuleLint.warned) == warned - assert len(mocked_ModuleLint.failed) == failed + 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,