diff --git a/.github/actions/create-lint-wf/action.yml b/.github/actions/create-lint-wf/action.yml index 69d42c5d07..4674d82daf 100644 --- a/.github/actions/create-lint-wf/action.yml +++ b/.github/actions/create-lint-wf/action.yml @@ -76,6 +76,15 @@ runs: run: find nf-core-testpipeline -not -path '*/.git/*' -type f -exec sed -i 's/\/\/ includeConfig/includeConfig/' {} \; working-directory: create-lint-wf + # Add a placeholder metro map and enable manifest.diagram + - name: add metro map + shell: bash + run: | + mkdir -p nf-core-testpipeline/docs/images + touch nf-core-testpipeline/docs/images/metro_map.svg + sed -i 's|// diagram|diagram|' nf-core-testpipeline/nextflow.config + working-directory: create-lint-wf + # Replace zenodo.XXXXXX to pass readme linting - name: replace zenodo.XXXXXX shell: bash diff --git a/.github/workflows/create-test-lint-wf-template.yml b/.github/workflows/create-test-lint-wf-template.yml index 9c101fbdd5..a2b131d697 100644 --- a/.github/workflows/create-test-lint-wf-template.yml +++ b/.github/workflows/create-test-lint-wf-template.yml @@ -164,6 +164,14 @@ jobs: run: find my-prefix-testpipeline -type f -exec sed -i 's/\/\/ includeConfig/includeConfig/' {} \; working-directory: create-test-lint-wf + # Add a placeholder metro map and enable manifest.diagram + - name: add metro map + run: | + mkdir -p my-prefix-testpipeline/docs/images + touch my-prefix-testpipeline/docs/images/metro_map.svg + sed -i 's|// diagram|diagram|' my-prefix-testpipeline/nextflow.config + working-directory: create-test-lint-wf + # Replace zenodo.XXXXXX to pass readme linting - name: replace zenodo.XXXXXX run: find my-prefix-testpipeline -type f -exec sed -i 's/zenodo.XXXXXX/zenodo.123456/g' {} \; diff --git a/CHANGELOG.md b/CHANGELOG.md index e6a62cdaa7..21c8afb9f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,12 +6,15 @@ ### Linting +- Warn if `manifest.diagram` is not set, and fail if it is not a relative path to an existing image file ([#4460](https://github.com/nf-core/tools/pull/4460)) + ### Modules ### Subworkflows ### Template +- Add a commented-out `manifest.diagram` to `nextflow.config`, for the pipeline metro map ([#4460](https://github.com/nf-core/tools/pull/4460)) - git-ignore nf-test files ([#4461](https://github.com/nf-core/tools/pull/4461)) ### Version updates diff --git a/nf_core/pipeline-template/nextflow.config b/nf_core/pipeline-template/nextflow.config index 34806e4729..36b6dab492 100644 --- a/nf_core/pipeline-template/nextflow.config +++ b/nf_core/pipeline-template/nextflow.config @@ -313,6 +313,8 @@ manifest { nextflowVersion = '!>=25.10.4' version = '{{ version }}' doi = '' + // TODO nf-core: Make a metro map and add relative path to the SVG below. See https://nf-co.re/docs/community/brand/workflow-schematics + // diagram = 'docs/images/metro_map.svg' } {% if nf_schema -%} diff --git a/nf_core/pipelines/lint/nextflow_config.py b/nf_core/pipelines/lint/nextflow_config.py index caea0fa352..fd219aba34 100644 --- a/nf_core/pipelines/lint/nextflow_config.py +++ b/nf_core/pipelines/lint/nextflow_config.py @@ -7,6 +7,9 @@ log = logging.getLogger(__name__) +# Image formats that Nextflow accepts for `manifest.diagram` +DIAGRAM_EXTENSIONS = {".svg", ".png", ".jpg", ".jpeg", ".gif", ".webp"} + def nextflow_config(self) -> dict[str, list[str]]: """Checks the pipeline configuration for required variables. @@ -68,11 +71,27 @@ def nextflow_config(self) -> dict[str, list[str]]: **The following variables throw warnings if missing:** * ``manifest.mainScript``: The filename of the main pipeline script (should be ``main.nf``) + * ``manifest.diagram`` + + * A relative path to a workflow diagram (metro map) for the pipeline, eg. ``docs/images/metro_map.svg`` + * Any SVG works, including hand-drawn ones. `nf-metro `_ + can generate one from a config file, if you'd like a hand. + * Requires Nextflow ``26.10.0`` or later, but is safe to set for older versions - they ignore it. + * If set, the value must be valid or the test **fails** (see below) + * ``timeline.file``, ``trace.file``, ``report.file``, ``dag.file`` * Default filenames for the timeline, trace and report * The DAG file path should end with ``.svg`` (If Graphviz is not installed, Nextflow will generate a ``.dot`` file instead) + **The following variables fail the test if they are set to an invalid value:** + + * ``manifest.diagram`` + + * Must be a relative path inside the pipeline (not a URL, absolute path or ``../``) + * Must be one of the image formats that Nextflow accepts: ``.svg``, ``.png``, ``.jpg``, ``.jpeg``, ``.gif``, ``.webp`` + * Must point at a file that exists in the pipeline + **The following variables are depreciated and fail the test if they are still present:** * ``params.version``: The old method for specifying the pipeline version. Replaced by ``manifest.version`` @@ -150,6 +169,7 @@ def nextflow_config(self) -> dict[str, list[str]]: # Throw a warning if these are missing config_warn = [ ["manifest.mainScript"], + ["manifest.diagram"], ["timeline.file"], ["trace.file"], ["report.file"], @@ -272,6 +292,21 @@ def _config_has_key(key: str) -> bool: else: failed.append(f"Config ``dag.file`` did not end with ``{default_dag_format}``") + # Check that manifest.diagram points at a file that exists in the pipeline + diagram = manifest.get("diagram", "") + if diagram and "manifest.diagram" not in ignore_configs: + if re.match(r"^\w+://", diagram) or Path(diagram).is_absolute() or ".." in Path(diagram).parts: + failed.append(f"Config ``manifest.diagram`` should be a relative path inside the pipeline: ``{diagram}``") + elif Path(diagram).suffix.lower() not in DIAGRAM_EXTENSIONS: + failed.append( + f"Config ``manifest.diagram`` is not a supported image format " + f"({', '.join(sorted(DIAGRAM_EXTENSIONS))}): ``{diagram}``" + ) + elif (Path(self.wf_path) / diagram).is_file(): + passed.append(f"Config ``manifest.diagram`` file found: ``{diagram}``") + else: + failed.append(f"Config ``manifest.diagram`` file not found: ``{diagram}``") + # Check that the minimum nextflowVersion is set properly nextflow_version = manifest.get("nextflowVersion", "") if nextflow_version: diff --git a/tests/pipelines/lint/test_nextflow_config.py b/tests/pipelines/lint/test_nextflow_config.py index 493aa49a76..e9c6ccae81 100644 --- a/tests/pipelines/lint/test_nextflow_config.py +++ b/tests/pipelines/lint/test_nextflow_config.py @@ -7,18 +7,33 @@ from ..test_lint import TestLint +# The template ships `manifest.diagram` commented out, so a new pipeline always warns about it +MISSING_DIAGRAM_WARNING = "Config variable not found: `manifest.diagram`" + class TestLintNextflowConfig(TestLint): def setUp(self) -> None: super().setUp() self.new_pipeline = self._make_pipeline_copy() + def _set_manifest_diagram(self, value: str) -> None: + """Uncomment the templated `manifest.diagram` line and set it to `value`.""" + nf_conf_file = Path(self.new_pipeline) / "nextflow.config" + content = nf_conf_file.read_text() + assert "// diagram" in content + nf_conf_file.write_text(content.replace("// diagram", f"diagram = '{value}' //")) + + def _lint_new_pipeline(self) -> dict: + lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) + lint_obj.load_pipeline_config() + return lint_obj.nextflow_config() + def test_nextflow_config_example_pass(self): """Tests that config variable existence test works with good pipeline example""" self.lint_obj.load_pipeline_config() result = self.lint_obj.nextflow_config() assert len(result["failed"]) == 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] def test_default_values_match(self): """Test that the default values in nextflow.config match the default values defined in the nextflow_schema.json.""" @@ -26,7 +41,7 @@ def test_default_values_match(self): lint_obj.load_pipeline_config() result = lint_obj.nextflow_config() assert len(result["failed"]) == 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] assert "Config default value correct: params.validate_params" in str(result["passed"]) def test_nextflow_config_bad_name_fail(self): @@ -37,7 +52,7 @@ def test_nextflow_config_bad_name_fail(self): lint_obj.nf_config["manifest"]["name"] = "bad_name" result = lint_obj.nextflow_config() assert len(result["failed"]) > 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] def test_nextflow_config_dev_in_release_mode_failed(self): """Tests that config variable existence test fails with dev version in release mode""" @@ -48,7 +63,7 @@ def test_nextflow_config_dev_in_release_mode_failed(self): lint_obj.nf_config["manifest"]["version"] = "dev_is_bad_name" result = lint_obj.nextflow_config() assert len(result["failed"]) > 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] def test_nextflow_config_missing_test_profile_failed(self): """Test failure if config file does not contain `test` profile.""" @@ -64,7 +79,7 @@ def test_nextflow_config_missing_test_profile_failed(self): lint_obj.load_pipeline_config() result = lint_obj.nextflow_config() assert len(result["failed"]) > 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] def test_default_values_fail(self): """Test linting fails if the default values in nextflow.config do not match the ones defined in the nextflow_schema.json.""" @@ -181,7 +196,7 @@ def test_default_values_float(self): lint_obj.load_pipeline_config() result = lint_obj.nextflow_config() assert len(result["failed"]) == 0 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] assert "Config default value correct: params.dummy" in str(result["passed"]) def test_default_values_float_fail(self): @@ -214,5 +229,59 @@ def test_default_values_float_fail(self): result = lint_obj.nextflow_config() assert len(result["failed"]) == 1 - assert len(result["warned"]) == 0 + assert result["warned"] == [MISSING_DIAGRAM_WARNING] assert "Config default value incorrect: `params.dummy" in str(result["failed"]) + + def test_manifest_diagram_pass(self): + """Test that a `manifest.diagram` pointing at an existing file passes.""" + diagram = self.new_pipeline / "docs" / "images" / "metro_map.svg" + diagram.parent.mkdir(parents=True, exist_ok=True) + diagram.write_text("") + self._set_manifest_diagram("docs/images/metro_map.svg") + + result = self._lint_new_pipeline() + assert len(result["failed"]) == 0 + assert len(result["warned"]) == 0 + assert "Config ``manifest.diagram`` file found: ``docs/images/metro_map.svg``" in result["passed"] + + def test_manifest_diagram_missing_file_fail(self): + """Test that a `manifest.diagram` pointing at a non-existent file fails.""" + self._set_manifest_diagram("docs/images/metro_map.svg") + + result = self._lint_new_pipeline() + assert "Config ``manifest.diagram`` file not found: ``docs/images/metro_map.svg``" in result["failed"] + + def test_manifest_diagram_bad_format_fail(self): + """Test that a `manifest.diagram` that is not a supported image format fails.""" + diagram = Path(self.new_pipeline) / "docs" / "images" / "metro_map.pdf" + diagram.parent.mkdir(parents=True, exist_ok=True) + diagram.write_text("not an image") + self._set_manifest_diagram("docs/images/metro_map.pdf") + + result = self._lint_new_pipeline() + assert ( + "Config ``manifest.diagram`` is not a supported image format " + "(.gif, .jpeg, .jpg, .png, .svg, .webp): ``docs/images/metro_map.pdf``" in result["failed"] + ) + + def test_manifest_diagram_url_fail(self): + """Test that a `manifest.diagram` set to a URL fails - it should be a relative path.""" + url = "https://example.com/metro_map.svg" + self._set_manifest_diagram(url) + + result = self._lint_new_pipeline() + assert ( + f"Config ``manifest.diagram`` should be a relative path inside the pipeline: ``{url}``" in result["failed"] + ) + + def test_manifest_diagram_absolute_path_fail(self): + """Test that an absolute `manifest.diagram` fails, instead of resolving outside the pipeline.""" + outside = Path(self.new_pipeline).parent / "metro_map.svg" + outside.write_text("") + self._set_manifest_diagram(str(outside)) + + result = self._lint_new_pipeline() + assert ( + f"Config ``manifest.diagram`` should be a relative path inside the pipeline: ``{outside}``" + in result["failed"] + )