From f5e80b203bf89be5ecc6076f45b7f01dbf7be2e6 Mon Sep 17 00:00:00 2001 From: Mohammad Alisafaee Date: Thu, 13 Apr 2023 02:13:38 +0200 Subject: [PATCH] feat(core): do not update template if dockerfile is modified --- renku/command/migrate.py | 9 +++---- renku/core/migration/migrate.py | 32 +++++++++++----------- renku/core/template/template.py | 15 ++++++----- renku/core/template/usecase.py | 15 +++++++---- renku/domain_model/template.py | 47 +++++++++++++++++++++++++++++++-- tests/core/test_template.py | 30 +++++++++++++++++++-- 6 files changed, 110 insertions(+), 38 deletions(-) diff --git a/renku/command/migrate.py b/renku/command/migrate.py index 63eae6edf0..baee994094 100644 --- a/renku/command/migrate.py +++ b/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 diff --git a/renku/core/migration/migrate.py b/renku/core/migration/migrate.py index 550ba346a2..993211ad2d 100644 --- a/renku/core/migration/migrate.py +++ b/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) diff --git a/renku/core/template/template.py b/renku/core/template/template.py index efd6ceb4e3..fae9847143 100644 --- a/renku/core/template/template.py +++ b/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 diff --git a/renku/core/template/usecase.py b/renku/core/template/usecase.py index 0792ee95a5..902a6ef810 100644 --- a/renku/core/template/usecase.py +++ b/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: diff --git a/renku/domain_model/template.py b/renku/domain_model/template.py index b2cf6a5664..19407d82f2 100644 --- a/renku/domain_model/template.py +++ b/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) + ) diff --git a/tests/core/test_template.py b/tests/core/test_template.py index b284aa5222..1ec4f1c19c 100644 --- a/tests/core/test_template.py +++ b/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