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

Author: mohammad-alisafaee

docs/spelling_wordlist.txt

@@ -287,6 +287,7 @@ versioned
 versioning
 Versioning
 vertices
+viewmodel
 vm
 wasDerivedFrom
 webhook

renku/command/migrate.py

@@ -131,9 +131,9 @@ def _dockerfile_migration_check():
         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(check_only=True)
 
     return {
         "automated_dockerfile_update": automated_dockerfile_update,
@@ -194,14 +194,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/command/view_model/template.py

@@ -156,6 +156,7 @@ def from_template(
             FileAction.KEEP: "Keep",
             FileAction.OVERWRITE: "Overwrite",
             FileAction.RECREATE: "Recreate deleted file",
+            FileAction.UPDATE_DOCKERFILE: "Update",
         }
 
         file_changes = [

renku/core/login.py

@@ -136,7 +136,7 @@ def login(endpoint: Optional[str], git_login: bool, yes: bool):
             raise errors.AuthenticationError(f"Invalid status code from server: {status_code} - {response.content}")
 
     access_token = response.json().get("access_token")
-    _store_token(parsed_endpoint.netloc, access_token)
+    store_token(parsed_endpoint.netloc, access_token)
 
     if git_login and repository:
         set_git_credential_helper(repository=cast("Repository", repository), hostname=parsed_endpoint.netloc)
@@ -170,8 +170,9 @@ def _get_url(parsed_endpoint, path, **query_args) -> str:
     return parsed_endpoint._replace(path=path, query=query).geturl()
 
 
-def _store_token(netloc, access_token):
-    set_value(section=CONFIG_SECTION, key=netloc, value=access_token, global_only=True)
+def store_token(hostname: str, access_token: str):
+    """Store access token for ``hostname``."""
+    set_value(section=CONFIG_SECTION, key=hostname, value=access_token, global_only=True)
     os.chmod(project_context.global_config_path, 0o600)
 
 
@@ -269,6 +270,11 @@ def _restore_git_remote(repository):
             communication.error(f"Cannot delete backup remote '{backup_remote}'")
 
 
+def has_credentials_for_hostname(hostname: str) -> bool:
+    """Check if credentials are set for the given hostname."""
+    return bool(_read_renku_token_for_hostname(hostname))
+
+
 @validate_arguments(config=dict(arbitrary_types_allowed=True))
 def credentials(command: str, hostname: Optional[str]):
     """Credentials helper for git."""

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(check_only=True)[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()
         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=False) -> 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(dockerfile_content=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,8 @@
     TemplateParameter,
     TemplatesManifest,
     TemplatesSource,
+    hash_template_file,
+    update_dockerfile_content,
 )
 from renku.infrastructure.repository import Repository
 
@@ -73,6 +74,7 @@ class FileAction(IntEnum):
     KEEP = 6
     OVERWRITE = 7
     RECREATE = 8
+    UPDATE_DOCKERFILE = 9
 
 
 def fetch_templates_source(source: Optional[str], reference: Optional[str]) -> TemplatesSource:
@@ -142,6 +144,7 @@ def copy_template_metadata_to_project():
         FileAction.KEEP: ("", "Keeping"),
         FileAction.OVERWRITE: ("copy", "Overwriting"),
         FileAction.RECREATE: ("copy", "Recreating deleted file"),
+        FileAction.UPDATE_DOCKERFILE: ("dockerfile", "Updating"),
     }
 
     for relative_path, action in get_sorted_actions(actions=actions).items():
@@ -161,6 +164,10 @@ def copy_template_metadata_to_project():
                 shutil.copy(source, destination, follow_symlinks=False)
             elif operation == "append":
                 destination.write_text(destination.read_text() + "\n" + source.read_text())
+            elif operation == "dockerfile":
+                update_dockerfile_content(source=source, destination=destination)
+            else:
+                raise errors.TemplateUpdateError(f"Unknown operation '{operation}'")
         except OSError as e:
             # TODO: Use a general cleanup strategy: https://github.com/SwissDataScienceCenter/renku-python/issues/736
             if cleanup:
@@ -211,7 +218,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 +227,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 +236,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 +255,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.UPDATE_DOCKERFILE
         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/core/util/git.py

@@ -94,7 +94,7 @@ def is_valid_git_repository(repository: Optional["Repository"]) -> bool:
         repository(Optional[Repository]): The repository to check.
 
     Returns:
-        bool: Whether or not this is a valid Git repository.
+        bool: Whether this is a valid Git repository.
 
     """
     return repository is not None and repository.head.is_valid()
@@ -648,13 +648,15 @@ def clone_renku_repository(
         progress: The GitProgress object (Default value = None).
         config(Optional[dict], optional): Set configuration for the project (Default value = None).
         raise_git_except: Whether to raise git exceptions (Default value = False).
-        checkout_revision: The revision to checkout after clone (Default value = None).
+        checkout_revision: The revision to check out after clone (Default value = None).
         use_renku_credentials(bool, optional): Whether to use Renku provided credentials (Default value = False).
         reuse_existing_repository(bool, optional): Whether to clone over an existing repository (Default value = False).
 
     Returns:
         The cloned repository.
     """
+    from renku.core.login import has_credentials_for_hostname
+
     parsed_url = parse_git_url(url)
 
     clone_options = None
@@ -665,7 +667,11 @@ def clone_renku_repository(
         git_url = str(absolute_path)
     elif parsed_url.scheme in ["http", "https"] and gitlab_token:
         git_url = get_oauth_url(url, gitlab_token)
-    elif parsed_url.scheme in ["http", "https"] and use_renku_credentials:
+    elif (
+        parsed_url.scheme in ["http", "https"]
+        and use_renku_credentials
+        and has_credentials_for_hostname(parsed_url.hostname)  # NOTE: Don't change remote URL if no credentials exist
+    ):
         clone_options = [f"--config credential.helper='!renku credentials --hostname {parsed_url.hostname}'"]
         deployment_hostname = deployment_hostname or parsed_url.hostname
         git_url = get_renku_repo_url(url, deployment_hostname=deployment_hostname, access_token=None)
@@ -727,7 +733,7 @@ def clone_repository(
         progress: The GitProgress object (Default value = None).
         config(Optional[dict], optional): Set configuration for the project (Default value = None).
         raise_git_except: Whether to raise git exceptions (Default value = False).
-        checkout_revision: The revision to checkout after clone (Default value = None).
+        checkout_revision: The revision to check out after clone (Default value = None).
         no_checkout(bool, optional): Whether to perform a checkout (Default value = False).
         clean(bool, optional): Whether to require the target folder to be clean (Default value = False).
         clone_options(List[str], optional): Additional clone options (Default value = None).
@@ -775,7 +781,7 @@ def check_and_reuse_existing_repository() -> Optional["Repository"]:
             if remote and have_same_remote(remote.url, url):
                 repository.reset(hard=True)
                 repository.fetch(all=True, tags=True)
-                # NOTE: By default we checkout remote repository's HEAD since the local HEAD might not point to
+                # NOTE: By default we check out remote repository's HEAD since the local HEAD might not point to
                 # the default branch.
                 default_checkout_revision = checkout_revision or "origin/HEAD"
                 repository.checkout(default_checkout_revision)