feat(core): do not update template if dockerfile is modified

renku/command/migrate.py

@@ -219,9 +219,9 @@ def _dockerfile_migration_check() -> DockerfileStatusResult:
         Dictionary of Dockerfile migration status.
     """
     from renku import __version__
-    from renku.core.migration.migrate import is_docker_update_possible
+    from renku.core.migration.migrate import update_dockerfile
 
-    automated_dockerfile_update, newer_renku_available, dockerfile_renku_version = is_docker_update_possible()
+    automated_dockerfile_update, newer_renku_available, dockerfile_renku_version = update_dockerfile()
 
     return DockerfileStatusResult(
         automated_dockerfile_update=automated_dockerfile_update,
@@ -282,14 +282,13 @@ def _check_project():
             # NOTE: v10 migration not done
             return MIGRATION_REQUIRED
 
-    # NOTE: ``project.automated_update`` is deprecated and we always allow template update for a project
+    # NOTE: ``project.automated_update`` is deprecated. We always allow template update for a project
     status = AUTOMATED_TEMPLATE_UPDATE_SUPPORTED
 
     if check_for_template_update(project_context.project)[0]:
         status |= TEMPLATE_UPDATE_POSSIBLE
-    if is_docker_update_possible()[0]:
+    if is_docker_update_possible():
         status |= DOCKERFILE_UPDATE_POSSIBLE
-
     if is_migration_required():
         return status | MIGRATION_REQUIRED
 

renku/core/migration/migrate.py

@@ -23,15 +23,15 @@
 public ``migrate`` function that accepts a ``MigrationContext`` as its argument.
 
 When executing a migration, the migration file is imported as a module and the
-``migrate`` function is executed. Migration version is checked against the Renku
-project version and any migration which has a higher version is applied to the
-project.
+``migrate`` function is executed. Renku checks project's metadata version and
+applies any migration that has a higher version to the project.
 """
 
 import importlib
 import re
 import shutil
 from pathlib import Path
+from typing import Optional, Tuple
 
 from packaging.version import Version
 
@@ -47,14 +47,13 @@
 from renku.core.interface.project_gateway import IProjectGateway
 from renku.core.migration.models.migration import MigrationContext, MigrationType
 from renku.core.migration.utils import is_using_temporary_datasets_path, read_project_version
-from renku.core.template.usecase import update_dockerfile_checksum
+from renku.core.template.usecase import calculate_dockerfile_checksum, update_dockerfile_checksum
 from renku.core.util import communication
 from renku.core.util.metadata import (
     is_renku_project,
     read_renku_version_from_dockerfile,
     replace_renku_version_in_dockerfile,
 )
-from renku.core.util.os import hash_string
 from renku.domain_model.project import ProjectTemplateMetadata
 from renku.domain_model.project_context import project_context
 
@@ -84,9 +83,9 @@ def is_project_unsupported():
     return is_renku_project() and get_project_version() > SUPPORTED_PROJECT_VERSION
 
 
-def is_docker_update_possible():
+def is_docker_update_possible() -> bool:
     """Check if the Dockerfile can be updated to a new version of renku-python."""
-    return _update_dockerfile(check_only=True)
+    return update_dockerfile()[0]
 
 
 @inject.autoparams("project_gateway")
@@ -141,15 +140,15 @@ def migrate_project(
             template_updated = _update_template()
         except TemplateUpdateError:
             raise
-        except (Exception, BaseException) as e:
+        except Exception as e:
             raise TemplateUpdateError("Couldn't update from template.") from e
 
     if not skip_docker_update:
         try:
-            docker_updated, _, _ = _update_dockerfile()
+            docker_updated, _, _ = update_dockerfile(check_only=False)
         except DockerfileUpdateError:
             raise
-        except (Exception, BaseException) as e:
+        except Exception as e:
             raise DockerfileUpdateError("Couldn't update renku version in Dockerfile.") from e
 
     if skip_migrations:
@@ -194,13 +193,12 @@ def _remove_untracked_renku_files(metadata_path):
         shutil.rmtree(path, ignore_errors=True)
 
 
-@inject.autoparams()
-def _update_template(project_gateway: IProjectGateway) -> bool:
+def _update_template() -> bool:
     """Update local files from the remote template."""
     from renku.core.template.usecase import update_template
 
     try:
-        project = project_gateway.get_project()
+        project = project_context.project
     except ValueError:
         # NOTE: Old project, we don't know the status until it is migrated
         return False
@@ -211,15 +209,13 @@ def _update_template(project_gateway: IProjectGateway) -> bool:
     return bool(update_template(interactive=False, force=False, dry_run=False))
 
 
-def _update_dockerfile(check_only=False):
+def update_dockerfile(check_only=True) -> Tuple[bool, Optional[bool], Optional[str]]:
     """Update the dockerfile to the newest version of renku."""
     from renku import __version__
 
     if not project_context.dockerfile_path.exists():
         return False, None, None
 
-    communication.echo("Updating dockerfile...")
-
     with open(project_context.dockerfile_path) as f:
         dockerfile_content = f.read()
 
@@ -238,8 +234,10 @@ def _update_dockerfile(check_only=False):
     if check_only:
         return True, True, str(docker_version)
 
+    communication.echo("Updating dockerfile...")
+
     new_content = replace_renku_version_in_dockerfile(dockerfile_content=dockerfile_content, version=__version__)
-    new_checksum = hash_string(new_content)
+    new_checksum = calculate_dockerfile_checksum(new_content)
 
     try:
         update_dockerfile_checksum(new_checksum=new_checksum)

renku/core/template/template.py

@@ -29,7 +29,6 @@
 from renku.core import errors
 from renku.core.util import communication
 from renku.core.util.git import clone_repository
-from renku.core.util.os import hash_file
 from renku.core.util.util import to_semantic_version, to_string
 from renku.domain_model.project_context import project_context
 from renku.domain_model.template import (
@@ -40,6 +39,7 @@
     TemplateParameter,
     TemplatesManifest,
     TemplatesSource,
+    hash_template_file,
 )
 from renku.infrastructure.repository import Repository
 
@@ -211,7 +211,7 @@ def get_action_for_initialize(relative_path: str, destination: Path) -> FileActi
 
     def get_action_for_set(relative_path: str, destination: Path, new_checksum: Optional[str]) -> FileAction:
         """Decide what to do with a template file."""
-        current_checksum = hash_file(destination)
+        current_checksum = hash_template_file(relative_path=relative_path, absolute_path=destination)
 
         if not destination.exists():
             return FileAction.CREATE
@@ -220,8 +220,6 @@ def get_action_for_set(relative_path: str, destination: Path, new_checksum: Opti
         elif interactive:
             overwrite = communication.confirm(f"Overwrite {relative_path}?", default=True)
             return FileAction.OVERWRITE if overwrite else FileAction.KEEP
-        elif relative_path == "Dockerfile" and not is_dockerfile_updated_by_user():
-            return FileAction.OVERWRITE
         elif should_keep(relative_path):
             return FileAction.KEEP
         else:
@@ -231,7 +229,7 @@ def get_action_for_update(
         relative_path: str, destination: Path, old_checksum: Optional[str], new_checksum: Optional[str]
     ) -> FileAction:
         """Decide what to do with a template file."""
-        current_checksum = hash_file(destination)
+        current_checksum = hash_template_file(relative_path=relative_path, absolute_path=destination)
         local_changes = current_checksum != old_checksum
         remote_changes = new_checksum != old_checksum
         file_exists = destination.exists()
@@ -250,8 +248,11 @@ def get_action_for_update(
                 return FileAction.OVERWRITE if overwrite else FileAction.KEEP
         elif not remote_changes:
             return FileAction.IGNORE_UNCHANGED_REMOTE
-        elif relative_path == "Dockerfile" and not is_dockerfile_updated_by_user():
-            return FileAction.OVERWRITE
+        elif relative_path == "Dockerfile":
+            if is_dockerfile_updated_by_user():
+                raise errors.TemplateUpdateError("Can't update template as Dockerfile was locally changed.")
+            else:
+                return FileAction.OVERWRITE
         elif file_deleted or local_changes:
             if relative_path in immutable_files:
                 # NOTE: There are local changes in a file that should not be changed by users, and the file was

renku/core/template/usecase.py

@@ -41,11 +41,16 @@
 )
 from renku.core.util import communication
 from renku.core.util.metadata import is_renku_project, replace_renku_version_in_dockerfile
-from renku.core.util.os import hash_file, hash_string
 from renku.core.util.tabulate import tabulate
 from renku.domain_model.project import Project
 from renku.domain_model.project_context import project_context
-from renku.domain_model.template import RenderedTemplate, Template, TemplateMetadata, TemplatesSource
+from renku.domain_model.template import (
+    RenderedTemplate,
+    Template,
+    TemplateMetadata,
+    TemplatesSource,
+    calculate_dockerfile_checksum,
+)
 from renku.infrastructure.repository import DiffLineChangeType, Repository
 
 
@@ -89,7 +94,7 @@ def is_dockerfile_updated_by_user() -> bool:
         return False
 
     original_checksum = read_template_checksum().get("Dockerfile")
-    current_checksum = hash_file(dockerfile)
+    current_checksum = calculate_dockerfile_checksum(dockerfile=dockerfile)
 
     if original_checksum == current_checksum:  # Dockerfile was never updated
         return False
@@ -99,7 +104,7 @@ def is_dockerfile_updated_by_user() -> bool:
     original_renku_version = metadata.get("__renku_version__")
 
     original_dockerfile_content = replace_renku_version_in_dockerfile(dockerfile.read_text(), original_renku_version)
-    original_calculated_checksum = hash_string(original_dockerfile_content)
+    original_calculated_checksum = calculate_dockerfile_checksum(dockerfile_content=original_dockerfile_content)
 
     if original_checksum == original_calculated_checksum:
         return False
@@ -186,7 +191,7 @@ def set_template(
 
 @validate_arguments(config=dict(arbitrary_types_allowed=True))
 def update_template(force: bool, interactive: bool, dry_run: bool) -> Optional[TemplateChangeViewModel]:
-    """Update project's template if possible. Return True if updated."""
+    """Update project's template if possible. Return corresponding viewmodel if updated."""
     template_metadata = TemplateMetadata.from_project(project=project_context.project)
 
     if not template_metadata.source:

renku/domain_model/template.py

@@ -28,7 +28,7 @@
 
 from renku.core import errors
 from renku.core.constant import RENKU_HOME
-from renku.core.util.os import get_safe_relative_path, hash_file
+from renku.core.util.os import get_safe_relative_path, hash_file, hash_string
 from renku.core.util.util import to_string
 
 if TYPE_CHECKING:
@@ -379,7 +379,10 @@ def __init__(self, path: Path, template: Template, metadata: Dict[str, Any]):
         self.path: Path = path
         self.template: Template = template
         self.metadata: Dict[str, Any] = metadata
-        self.checksums: Dict[str, Optional[str]] = {f: hash_file(self.path / f) for f in self.get_files()}
+        # TODO: Dockerfile checksum is calculated differently
+        self.checksums: Dict[str, Optional[str]] = {
+            f: hash_template_file(relative_path=f, absolute_path=self.path / f) for f in self.get_files()
+        }
 
     def get_files(self) -> Generator[str, None, None]:
         """Return all files in a rendered renku template."""
@@ -586,3 +589,43 @@ def update(self, template: Template):
         self.metadata["__template_id__"] = template.id
         self.metadata["__automated_update__"] = template.allow_update
         self.immutable_files = template.immutable_files
+
+
+def get_renku_section_from_dockerfile(content: str) -> str:
+    """Return the Renku-specific section of the Dockerfile or the whole Dockerfile if it doesn't exist.
+
+    NOTE: We ignore whitespaces at the end of the lines and empty lines.
+    """
+    lines = [line.rstrip() for line in content.splitlines() if line.strip()]
+    start = end = -1
+    for index, line in enumerate(lines):
+        if line.startswith("#        Renku-specific section - DO NOT MODIFY        #"):
+            start = index
+        elif line.endswith("#              End Renku-specific section              #"):
+            end = index
+            break
+
+    return "\n".join(lines[start:end]) if 0 <= start <= end else content
+
+
+def calculate_dockerfile_checksum(
+    *, dockerfile: Optional[Path] = None, dockerfile_content: Optional[str] = None
+) -> str:
+    """Calculate checksum for the given file or content."""
+    if not dockerfile and not dockerfile_content:
+        raise errors.ParameterError("Either Dockerfile or its content must be passed")
+    elif dockerfile and dockerfile_content:
+        raise errors.ParameterError("Cannot pass both Dockerfile and its content")
+
+    content = dockerfile_content if dockerfile_content is not None else dockerfile.read_text()  # type: ignore
+    renku_section = get_renku_section_from_dockerfile(content)
+    return hash_string(renku_section)
+
+
+def hash_template_file(*, relative_path: Union[Path, str], absolute_path: Union[Path, str]) -> Optional[str]:
+    """Use proper hash on a template file."""
+    return (
+        calculate_dockerfile_checksum(dockerfile=Path(absolute_path))
+        if str(relative_path) == "Dockerfile"
+        else hash_file(absolute_path)
+    )

tests/core/test_template.py

@@ -186,16 +186,30 @@ def test_get_file_actions_for_update(project_with_template, rendered_template_wi
     assert FileAction.OVERWRITE == actions[remotely_modified]
 
 
+def test_template_set_with_locally_modified_dockerfile(
+    project_with_template, rendered_template_with_update, with_injection
+):
+    """Test setting template with a locally modified Dockerfile will overwrite the Dockerfile."""
+    (project_context.path / "Dockerfile").write_text("Local modification")
+
+    with with_injection():
+        actions = get_file_actions(
+            rendered_template=rendered_template_with_update, template_action=TemplateAction.SET, interactive=False
+        )
+
+    assert FileAction.OVERWRITE == actions["Dockerfile"]
+
+
 def test_update_with_locally_modified_file(project_with_template, rendered_template_with_update, with_injection):
     """Test a locally modified file that is remotely updated won't change."""
-    (project_context.path / "Dockerfile").write_text("Local modification")
+    (project_context.path / "requirements.txt").write_text("Local modification")
 
     with with_injection():
         actions = get_file_actions(
             rendered_template=rendered_template_with_update, template_action=TemplateAction.UPDATE, interactive=False
         )
 
-    assert FileAction.KEEP == actions["Dockerfile"]
+    assert FileAction.KEEP == actions["requirements.txt"]
 
 
 def test_update_with_locally_deleted_file(project_with_template, rendered_template_with_update, with_injection):
@@ -210,6 +224,18 @@ def test_update_with_locally_deleted_file(project_with_template, rendered_templa
     assert FileAction.DELETED == actions["requirements.txt"]
 
 
+def test_update_with_locally_modified_dockerfile(project_with_template, rendered_template_with_update, with_injection):
+    """Test a locally modified Dockerfile that is remotely updated will raise an exception."""
+    (project_context.path / "Dockerfile").write_text("Local modification")
+
+    with pytest.raises(
+        errors.TemplateUpdateError, match="Can't update template as Dockerfile was locally changed."
+    ), with_injection():
+        get_file_actions(
+            rendered_template=rendered_template_with_update, template_action=TemplateAction.UPDATE, interactive=False
+        )
+
+
 @pytest.mark.parametrize("delete", [False, True])
 def test_update_with_locally_changed_immutable_file(
     project_with_template, rendered_template_with_update, with_injection, delete