From 5d00842f9c4ffd97c1a846f0ce95324b5b02b702 Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Fri, 9 Jun 2023 12:09:33 +0100 Subject: [PATCH 1/5] Add ability to select custom registry when linting modules Changes: - Adds --registry parameter to nf-core modules lint - Will check container definition via this registry - Default to quay.io --- CHANGELOG.md | 1 + nf_core/__main__.py | 6 +++++- nf_core/modules/lint/__init__.py | 16 +++++++++------- nf_core/modules/lint/main_nf.py | 24 ++++++++++++++++-------- tests/modules/lint.py | 10 ++++++++++ 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6599be57..407e04ed4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Fixed the Slack report to include the pipeline name ([#2291](https://github.com/nf-core/tools/pull/2291)) - Fix link in the MultiQC report to point to exact version of output docs ([#2298](https://github.com/nf-core/tools/pull/2298)) - Fix parsing of container directive when it is not typical nf-core format ([#2306](https://github.com/nf-core/tools/pull/2306)) +- Add ability to specify custom registry for linting modules, defaults to quay.io ### Download diff --git a/nf_core/__main__.py b/nf_core/__main__.py index 6d6ded471..00fba3631 100644 --- a/nf_core/__main__.py +++ b/nf_core/__main__.py @@ -807,6 +807,9 @@ def create_test_yml(ctx, tool, run_tests, output, force, no_prompts): @click.pass_context @click.argument("tool", type=str, required=False, metavar=" or ") @click.option("-d", "--dir", type=click.Path(exists=True), default=".", metavar="") +@click.option( + "-r", "--registry", type=str, metavar="", default="quay.io", help="Registry to use for containers" +) @click.option("-k", "--key", type=str, metavar="", multiple=True, help="Run only these lint tests") @click.option("-a", "--all", is_flag=True, help="Run on all modules") @click.option("-w", "--fail-warned", is_flag=True, help="Convert warn tests to failures") @@ -821,7 +824,7 @@ def create_test_yml(ctx, tool, run_tests, output, force, no_prompts): ) @click.option("--fix-version", is_flag=True, help="Fix the module version if a newer version is available") def lint( - ctx, tool, dir, key, all, fail_warned, local, passed, sort_by, fix_version + ctx, tool, dir, registry, key, all, fail_warned, local, passed, sort_by, fix_version ): # pylint: disable=redefined-outer-name """ Lint one or more modules in a directory. @@ -846,6 +849,7 @@ def lint( ) module_lint.lint( module=tool, + registry=registry, key=key, all_modules=all, print_results=True, diff --git a/nf_core/modules/lint/__init__.py b/nf_core/modules/lint/__init__.py index 24d673b1c..f288f1776 100644 --- a/nf_core/modules/lint/__init__.py +++ b/nf_core/modules/lint/__init__.py @@ -145,6 +145,7 @@ def get_all_lint_tests(is_pipeline): def lint( self, module=None, + registry="quay.io", key=(), all_modules=False, print_results=True, @@ -227,11 +228,11 @@ def lint( # Lint local modules if local and len(local_modules) > 0: - self.lint_modules(local_modules, local=True, fix_version=fix_version) + self.lint_modules(local_modules, registry=registry, local=True, fix_version=fix_version) # Lint nf-core modules if len(remote_modules) > 0: - self.lint_modules(remote_modules, local=False, fix_version=fix_version) + self.lint_modules(remote_modules, registry=registry, local=False, fix_version=fix_version) if print_results: self._print_results(show_passed=show_passed, sort_by=sort_by) @@ -264,12 +265,13 @@ def filter_tests_by_key(self, key): # If -k supplied, only run these tests self.lint_tests = [k for k in self.lint_tests if k in key] - def lint_modules(self, modules, local=False, fix_version=False): + def lint_modules(self, modules, registry, local=False, fix_version=False): """ Lint a list of modules Args: modules ([NFCoreModule]): A list of module objects + registry (str): The container registry to use. Should be quay.io in most situations. local (boolean): Whether the list consist of local or nf-core modules fix_version (boolean): Fix the module version if a newer version is available """ @@ -290,9 +292,9 @@ def lint_modules(self, modules, local=False, fix_version=False): for mod in modules: progress_bar.update(lint_progress, advance=1, test_name=mod.module_name) - self.lint_module(mod, progress_bar, local=local, fix_version=fix_version) + self.lint_module(mod, progress_bar, registry=registry, local=local, fix_version=fix_version) - def lint_module(self, mod, progress_bar, local=False, fix_version=False): + def lint_module(self, mod, progress_bar, registry, local=False, fix_version=False): """ Perform linting on one module @@ -311,7 +313,7 @@ def lint_module(self, mod, progress_bar, local=False, fix_version=False): # Only check the main script in case of a local module if local: - self.main_nf(mod, fix_version, progress_bar) + self.main_nf(mod, fix_version, registry, progress_bar) self.passed += [LintResult(mod, *m) for m in mod.passed] warned = [LintResult(mod, *m) for m in (mod.warned + mod.failed)] if not self.fail_warned: @@ -323,7 +325,7 @@ def lint_module(self, mod, progress_bar, local=False, fix_version=False): else: for test_name in self.lint_tests: if test_name == "main_nf": - getattr(self, test_name)(mod, fix_version, progress_bar) + getattr(self, test_name)(mod, fix_version, registry, progress_bar) else: getattr(self, test_name)(mod) diff --git a/nf_core/modules/lint/main_nf.py b/nf_core/modules/lint/main_nf.py index 73d1df365..18d95bd37 100644 --- a/nf_core/modules/lint/main_nf.py +++ b/nf_core/modules/lint/main_nf.py @@ -17,7 +17,7 @@ log = logging.getLogger(__name__) -def main_nf(module_lint_object, module, fix_version, progress_bar): +def main_nf(module_lint_object, module, fix_version, registry, progress_bar): """ Lint a ``main.nf`` module file @@ -121,7 +121,7 @@ def main_nf(module_lint_object, module, fix_version, progress_bar): module.passed.append(("main_nf_script_outputs", "Process 'output' block found", module.main_nf)) # Check the process definitions - if check_process_section(module, process_lines, fix_version, progress_bar): + if check_process_section(module, process_lines, registry, fix_version, progress_bar): module.passed.append(("main_nf_container", "Container versions match", module.main_nf)) else: module.warned.append(("main_nf_container", "Container versions do not match", module.main_nf)) @@ -209,12 +209,20 @@ def check_when_section(self, lines): self.passed.append(("when_condition", "when: condition is unchanged", self.main_nf)) -def check_process_section(self, lines, fix_version, progress_bar): - """ - Lint the section of a module between the process definition +def check_process_section(self, lines, registry, fix_version, progress_bar): + """Lint the section of a module between the process definition and the 'input:' definition Specifically checks for correct software versions and containers + + Args: + lines (List[str]): Content of process. + registry (str): Base Docker registry for containers. Typically quay.io. + fix_version (bool): Fix software version + progress_bar (ProgressBar): Progress bar to update. + + Returns: + Optional[bool]: True if singularity and docker containers match, False otherwise. If process definition does not exist, None. """ # Check that we have a process section if len(lines) == 0: @@ -288,7 +296,7 @@ def check_process_section(self, lines, fix_version, progress_bar): else: self.failed.append(("docker_tag", "Unable to parse docker tag", self.main_nf)) docker_tag = None - if l.startswith("quay.io/"): + if l.startswith(registry): l_stripped = re.sub(r"\W+$", "", l) self.failed.append( ( @@ -303,7 +311,7 @@ def check_process_section(self, lines, fix_version, progress_bar): # Guess if container name is simple one (e.g. nfcore/ubuntu:20.04) # If so, add quay.io as default container prefix if l.count("/") == 1 and l.count(":") == 1: - l = "quay.io/" + l + l = "/".join([registry, l]).replace("//", "/") url = urlparse(l.split("'")[0]) # lint double quotes @@ -326,7 +334,7 @@ def check_process_section(self, lines, fix_version, progress_bar): ) # lint more than one container in the same line - if ("https://containers" in l or "https://depot" in l) and ("biocontainers/" in l or "quay.io/" in l): + if ("https://containers" in l or "https://depot" in l) and ("biocontainers/" in l or l.startswith(registry)): self.warned.append( ( "container_links", diff --git a/tests/modules/lint.py b/tests/modules/lint.py index b7aaf610c..8e692e8ca 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -82,6 +82,16 @@ def test_modules_lint_multiple_remotes(self): assert len(module_lint.warned) >= 0 +def test_modules_lint_registry(self): + """Test linting the TrimGalore! module""" + self.mods_install.install("samtools") + module_lint = nf_core.modules.ModuleLint(dir=self.pipeline_dir) + module_lint.lint(print_results=False, registry="public.ecr.aws", module="samtools") + assert len(module_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" + assert len(module_lint.passed) > 0 + assert len(module_lint.warned) >= 0 + + def test_modules_lint_patched_modules(self): """ Test creating a patch file and applying it to a new version of the the files From 9b70efb5d1ce2d64ac770bebdff8c4bef827d1ec Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Mon, 12 Jun 2023 10:34:01 +0100 Subject: [PATCH 2/5] Quick test update --- tests/modules/lint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/modules/lint.py b/tests/modules/lint.py index 8e692e8ca..66da0019d 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -83,7 +83,7 @@ def test_modules_lint_multiple_remotes(self): def test_modules_lint_registry(self): - """Test linting the TrimGalore! module""" + """Test linting the samtools module and alternative registry""" self.mods_install.install("samtools") module_lint = nf_core.modules.ModuleLint(dir=self.pipeline_dir) module_lint.lint(print_results=False, registry="public.ecr.aws", module="samtools") From 464f0f75856ecedf3776d45c4d11a2d6ef14d3d5 Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Mon, 12 Jun 2023 11:04:02 +0100 Subject: [PATCH 3/5] CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 407e04ed4..be0ab9828 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ - Fixed the Slack report to include the pipeline name ([#2291](https://github.com/nf-core/tools/pull/2291)) - Fix link in the MultiQC report to point to exact version of output docs ([#2298](https://github.com/nf-core/tools/pull/2298)) - Fix parsing of container directive when it is not typical nf-core format ([#2306](https://github.com/nf-core/tools/pull/2306)) -- Add ability to specify custom registry for linting modules, defaults to quay.io +- Add ability to specify custom registry for linting modules, defaults to quay.io ([#2313](https://github.com/nf-core/tools/pull/2313)) ### Download From 5e52513f701540c78828d0c769a445e658b4cd21 Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Mon, 12 Jun 2023 11:04:30 +0100 Subject: [PATCH 4/5] Added 'quay.io' as default for lint_modules --- nf_core/modules/lint/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/modules/lint/__init__.py b/nf_core/modules/lint/__init__.py index f288f1776..ba3b3d18f 100644 --- a/nf_core/modules/lint/__init__.py +++ b/nf_core/modules/lint/__init__.py @@ -265,7 +265,7 @@ def filter_tests_by_key(self, key): # If -k supplied, only run these tests self.lint_tests = [k for k in self.lint_tests if k in key] - def lint_modules(self, modules, registry, local=False, fix_version=False): + def lint_modules(self, modules, registry="quay.io", local=False, fix_version=False): """ Lint a list of modules From 4fe8a043d686199240e16dd393d050626ed846a7 Mon Sep 17 00:00:00 2001 From: Adam Talbot Date: Mon, 12 Jun 2023 13:28:38 +0100 Subject: [PATCH 5/5] Add test for quay.io and public.aws.ecr explicitly for a direct comparison --- tests/modules/lint.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/modules/lint.py b/tests/modules/lint.py index 66da0019d..5b2c25080 100644 --- a/tests/modules/lint.py +++ b/tests/modules/lint.py @@ -90,6 +90,10 @@ def test_modules_lint_registry(self): assert len(module_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" assert len(module_lint.passed) > 0 assert len(module_lint.warned) >= 0 + module_lint.lint(print_results=False, registry="quay.io", module="samtools") + assert len(module_lint.failed) == 0, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" + assert len(module_lint.passed) > 0 + assert len(module_lint.warned) >= 0 def test_modules_lint_patched_modules(self):