Do not dump pipeline YAML if no migration is applied

STONEBLD-3438

* Compute file hash to detect if the pipeline is modified.
* New test is added, and by the way, the existing tests are refactored
  with fixtures for adding the new test.
* Test test_ensure_updates_to_pipeline_are_saved is renamed to reflect
  the test purpose specifically.

Author: tkdchen

src/pipeline_migration/migrate.py

@@ -11,7 +11,7 @@
 from dataclasses import dataclass, field
 from typing import Final, Any
 
-from pipeline_migration.utils import dump_yaml, load_yaml
+from pipeline_migration.utils import dump_yaml, load_yaml, file_checksum
 from pipeline_migration.registry import Container, Registry, ImageIndex
 from pipeline_migration.quay import QuayTagInfo, list_active_repo_tags
 from pipeline_migration.types import FilePath
@@ -113,9 +113,11 @@ def resolve_pipeline(pipeline_file: FilePath) -> Generator[FilePath, Any, None]:
 
     kind = origin_pipeline.get("kind")
     if kind == TEKTON_KIND_PIPELINE:
+        origin_checksum = file_checksum(pipeline_file)
         yield pipeline_file
-        pl_yaml = load_yaml(pipeline_file)
-        dump_yaml(pipeline_file, pl_yaml, style=yaml_style)
+        if file_checksum(pipeline_file) != origin_checksum:
+            pl_yaml = load_yaml(pipeline_file)
+            dump_yaml(pipeline_file, pl_yaml, style=yaml_style)
     elif kind == TEKTON_KIND_PIPELINE_RUN:
         spec = origin_pipeline.get("spec") or {}
         if "pipelineSpec" in spec:
@@ -124,10 +126,12 @@ def resolve_pipeline(pipeline_file: FilePath) -> Generator[FilePath, Any, None]:
             os.close(fd)
             pipeline = {"spec": spec["pipelineSpec"]}
             dump_yaml(temp_pipeline_file, pipeline)
+            origin_checksum = file_checksum(temp_pipeline_file)
             yield temp_pipeline_file
-            modified_pipeline = load_yaml(temp_pipeline_file)
-            spec["pipelineSpec"] = modified_pipeline["spec"]
-            dump_yaml(pipeline_file, origin_pipeline, style=yaml_style)
+            if file_checksum(temp_pipeline_file) != origin_checksum:
+                modified_pipeline = load_yaml(temp_pipeline_file)
+                spec["pipelineSpec"] = modified_pipeline["spec"]
+                dump_yaml(pipeline_file, origin_pipeline, style=yaml_style)
         elif "pipelineRef" in spec:
             # Pipeline definition can be referenced here, via either git-resolver or a name field
             # pointing to YAML file under the .tekton/.

src/pipeline_migration/utils.py

@@ -1,3 +1,4 @@
+import hashlib
 from dataclasses import dataclass, field
 from typing import Any
 
@@ -106,3 +107,8 @@ def load_yaml(yaml_file: FilePath) -> Any:
 def dump_yaml(yaml_file: FilePath, data: Any, style: YAMLStyle | None = None) -> None:
     with open(yaml_file, "w", encoding="utf-8") as f:
         create_yaml_obj(style).dump(data, f)
+
+
+def file_checksum(file_path: FilePath) -> str:
+    with open(file_path, "rb") as f:
+        return hashlib.sha256(f.read()).hexdigest()

tests/conftest.py

@@ -148,3 +148,34 @@ def pipeline_yaml_with_various_indent_styles(request, pipeline_yaml):
                         git clone https://git.host/project
                 """
             )
+        case _:
+            raise ValueError(f"Unexpected param {request.param}")
+
+
+@pytest.fixture
+def pipeline_run_yaml() -> str:
+    return dedent(
+        """\
+        apiVersion: tekton.dev/v1
+        kind: PipelineRun
+        metadata:
+          name: docker-build
+        spec:
+          pipelineSpec:
+            params:
+            tasks:
+            - name: clone
+            - name: build
+        """
+    )
+
+
+@pytest.fixture(params=["pipeline", "pipeline_run"])
+def pipeline_and_run_yaml(request, pipeline_yaml, pipeline_run_yaml) -> str:
+    match request.param:
+        case "pipeline":
+            return pipeline_yaml
+        case "pipeline_run":
+            return pipeline_run_yaml
+        case _:
+            raise ValueError(f"Unexpected param {request.param}")

tests/test_cli.py

@@ -23,7 +23,7 @@
     ensure_container,
 )
 
-from pipeline_migration.utils import YAMLStyle
+from pipeline_migration.utils import YAMLStyle, load_yaml, dump_yaml
 from tests.test_migrate import APP_IMAGE_REPO, TASK_BUNDLE_CLONE
 from tests.utils import generate_digest, generate_git_sha
 
@@ -296,6 +296,12 @@ def _subprocess_run(cmd, *args, **kwargs):
             assert pipeline_file == tb_upgrades[0]["packageFile"]
             migration_file = cmd[-2]
             assert Path(migration_file).read_bytes() in migration_steps
+
+            # Modify the pipeline as if a migration is applied
+            doc = load_yaml(pipeline_file)
+            doc["spec"]["tasks"] += {"name": "summary"}
+            dump_yaml(pipeline_file, doc)
+
             return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="")
 
         monkeypatch.setattr("subprocess.run", _subprocess_run)

tests/test_migrate.py

@@ -3,26 +3,31 @@
 import logging
 import subprocess
 from copy import deepcopy
+from pathlib import Path
 from textwrap import dedent
 from typing import Any, Final
+from unittest.mock import patch
 
 import responses
 import pytest
+from ruamel.yaml import YAML
 
 from pipeline_migration.migrate import (
     ANNOTATION_HAS_MIGRATION,
     ANNOTATION_IS_MIGRATION,
     ANNOTATION_TRUTH_VALUE,
-    LinkedMigrationsResolver,
     determine_task_bundle_upgrades_range,
     fetch_migration_file,
     IncorrectMigrationAttachment,
     InvalidRenovateUpgradesData,
+    LinkedMigrationsResolver,
     resolve_pipeline,
+    SimpleIterationResolver,
     TaskBundleMigration,
     TaskBundleUpgrade,
     TaskBundleUpgradesManager,
-    SimpleIterationResolver,
+    TEKTON_KIND_PIPELINE,
+    TEKTON_KIND_PIPELINE_RUN,
 )
 from pipeline_migration.quay import QuayTagInfo
 from pipeline_migration.registry import Container
@@ -268,63 +273,55 @@ def test_invalid_upgrade(self, data, expected):
 
 class TestResolvePipeline:
 
-    def test_resolve_from_a_pipeline_definition(self, tmp_path):
+    def test_resolve_from_a_pipeline_definition(self, pipeline_yaml, tmp_path):
         pipeline_file = tmp_path / "pl.yaml"
-        content = dedent(
-            """\
-            apiVersion: tekton.dev/v1
-            kind: Pipeline
-            metadata:
-                name: pl
-            spec:
-                params:
-                tasks:
-            """
-        )
-        pipeline_file.write_text(content)
-        with resolve_pipeline(pipeline_file):
-            pass
+        pipeline_file.write_text(pipeline_yaml)
+        with resolve_pipeline(pipeline_file) as f:
+            assert pipeline_yaml == Path(f).read_text()
 
-    def test_resolve_from_a_pipeline_run_definition(self, tmp_path):
+    def test_resolve_from_a_pipeline_run_definition(self, pipeline_run_yaml, tmp_path):
         pipeline_file = tmp_path / "plr.yaml"
-        content = dedent(
-            """\
-            apiVersion: tekton.dev/v1
-            kind: PipelineRun
-            metadata:
-                name: pl
-            spec:
-                pipelineSpec:
-                    params:
-                    tasks:
-            """
-        )
-        pipeline_file.write_text(content)
-        with resolve_pipeline(pipeline_file):
-            pass
+        pipeline_file.write_text(pipeline_run_yaml)
+        with resolve_pipeline(pipeline_file) as f:
+            resolved_pipeline = load_yaml(f)
+            assert "spec" in resolved_pipeline
+            pipeline_run = load_yaml(pipeline_file)
+            assert resolved_pipeline["spec"] == pipeline_run["spec"]["pipelineSpec"]
+
+    def test_updates_to_pipeline_are_dumped(self, pipeline_and_run_yaml, tmp_path):
+        pipeline_file = tmp_path / "file.yaml"
+        pipeline_file.write_text(pipeline_and_run_yaml)
 
-    def test_ensure_updates_to_pipeline_are_saved(self, tmp_path):
-        pipeline_file = tmp_path / "plr.yaml"
-        content = dedent(
-            """\
-            apiVersion: tekton.dev/v1
-            kind: PipelineRun
-            metadata:
-                name: pl
-            spec:
-                pipelineSpec:
-                    params:
-                    tasks:
-            """
-        )
-        pipeline_file.write_text(content)
         with resolve_pipeline(pipeline_file) as f:
             pl = load_yaml(f)
-            pl["spec"]["tasks"] = [{"name": "init"}]
+            pl["spec"]["tasks"].append({"name": "test"})
             dump_yaml(f, pl)
 
-        plr = load_yaml(pipeline_file)
-        assert plr["spec"]["pipelineSpec"]["tasks"] == [{"name": "init"}]
+        doc = load_yaml(pipeline_file)
+        if doc["kind"] == TEKTON_KIND_PIPELINE:
+            tasks = doc["spec"]["tasks"]
+        elif doc["kind"] == TEKTON_KIND_PIPELINE_RUN:
+            tasks = doc["spec"]["pipelineSpec"]["tasks"]
+        else:
+            raise ValueError(f"Unexpected kind {doc['kind']}")
+
+        assert tasks[-1]["name"] == "test"
+
+    @patch("pipeline_migration.migrate.dump_yaml")
+    def test_do_not_save_if_pipeline_is_not_modified(
+        self, mock_dump_yaml, pipeline_and_run_yaml, tmp_path
+    ):
+        pipeline_file = tmp_path / "plr.yaml"
+        pipeline_file.write_text(pipeline_and_run_yaml)
+
+        with resolve_pipeline(pipeline_file):
+            pass  # Nothing is changed
+
+        doc = YAML().load(pipeline_and_run_yaml)
+        if doc["kind"] == TEKTON_KIND_PIPELINE:
+            assert mock_dump_yaml.call_count == 0
+        elif doc["kind"] == TEKTON_KIND_PIPELINE_RUN:
+            assert mock_dump_yaml.call_count == 1
 
     def test_do_not_handle_pipelineref(self, tmp_path):
         pipeline_file = tmp_path / "plr.yaml"