From 1c677203268991e23e35c800d74ed8cc304e4515 Mon Sep 17 00:00:00 2001 From: Mohammad Alisafaee Date: Thu, 13 Apr 2023 02:13:38 +0200 Subject: [PATCH 1/2] feat(core): do not update template if dockerfile is modified --- docs/spelling_wordlist.txt | 1 + renku/command/migrate.py | 9 +-- renku/command/view_model/template.py | 1 + renku/core/login.py | 12 ++- renku/core/migration/migrate.py | 32 ++++---- renku/core/template/template.py | 22 ++++-- renku/core/template/usecase.py | 15 ++-- renku/core/util/git.py | 16 ++-- renku/domain_model/template.py | 76 ++++++++++++++++++- ...{test_integration_cli.py => test_clone.py} | 58 ++++++++++++-- tests/cli/test_template.py | 24 +++++- tests/core/test_template.py | 47 +++++++++++- tests/fixtures/templates.py | 20 ++++- 13 files changed, 276 insertions(+), 57 deletions(-) rename tests/cli/{test_integration_cli.py => test_clone.py} (70%) diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 1a1fce8ead..f4dfd9a11a 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -287,6 +287,7 @@ versioned versioning Versioning vertices +viewmodel vm wasDerivedFrom webhook diff --git a/renku/command/migrate.py b/renku/command/migrate.py index cebaa98465..5574184d8e 100644 --- a/renku/command/migrate.py +++ b/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() 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 diff --git a/renku/command/view_model/template.py b/renku/command/view_model/template.py index ea3ca0ae28..0a2f53c216 100644 --- a/renku/command/view_model/template.py +++ b/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.DOCKERFILE: "Update", } file_changes = [ diff --git a/renku/core/login.py b/renku/core/login.py index 7fb9058050..1004d23219 100644 --- a/renku/core/login.py +++ b/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.""" diff --git a/renku/core/migration/migrate.py b/renku/core/migration/migrate.py index 550ba346a2..f3ed7aab89 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(dockerfile_content=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..ccc715233f 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,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 + 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.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.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 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/core/util/git.py b/renku/core/util/git.py index a0d0a8d406..cd167454c8 100644 --- a/renku/core/util/git.py +++ b/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) diff --git a/renku/domain_model/template.py b/renku/domain_model/template.py index b2cf6a5664..8f02d8ab0b 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,9 @@ 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()} + 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 +588,73 @@ 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 find_renku_section(lines: List[str]) -> Tuple[int, int]: + """Return start and end line numbers of the Renku-specific section.""" + 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 start, end + + +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.""" + lines = [line.rstrip() for line in content.splitlines()] + start, end = find_renku_section(lines) + + if 0 <= start < end: + lines = lines[start:end] + lines = [line for line in lines if line] # NOTE: Remove empty lines + return "\n".join(lines) + else: + return content + + +def calculate_dockerfile_checksum( + *, dockerfile: Optional[Path] = None, dockerfile_content: Optional[str] = None +) -> str: + """Calculate checksum for the given file or content. + + NOTE: We ignore empty lines and whitespace characters at the end of the lines when calculating Dockerfile checksum + if it has Renku-specific section markers. + """ + 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 update_dockerfile_content(source: Path, destination: Path) -> None: + """Update the Renku-specific section of the destination Dockerfile with the one from the source Dockerfile.""" + source_lines = [line.rstrip() for line in source.read_text().splitlines()] + source_start, source_end = find_renku_section(source_lines) + + destination_lines = [line.rstrip() for line in destination.read_text().splitlines()] + destination_start, destination_end = find_renku_section(destination_lines) + + # NOTE: If source or destination Dockerfiles doesn't have Renku-specific section, we overwrite the whole file + if 0 <= source_start < source_end and 0 <= destination_start < destination_end: + destination_lines[destination_start:destination_end] = source_lines[source_start:source_end] + content = "\n".join(destination_lines) + destination.write_text(content) + else: + destination.write_text(source.read_text()) + + +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/cli/test_integration_cli.py b/tests/cli/test_clone.py similarity index 70% rename from tests/cli/test_integration_cli.py rename to tests/cli/test_clone.py index 9de285ee8b..8eb55d289a 100644 --- a/tests/cli/test_integration_cli.py +++ b/tests/cli/test_clone.py @@ -13,13 +13,16 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Integration tests for non-dataset CLI command.""" +"""Integration tests for ``renku clone`` command.""" + from pathlib import Path import pytest from renku.command.clone import project_clone_command +from renku.core.login import store_token from renku.core.util.contexts import chdir +from renku.infrastructure.repository import Repository from renku.ui.cli import cli from tests.utils import format_result_exception, retry_failed @@ -27,7 +30,7 @@ @pytest.mark.integration @retry_failed @pytest.mark.parametrize("url", ["https://gitlab.dev.renku.ch/renku-testing/project-9"]) -def test_renku_clone(runner, monkeypatch, url): +def test_clone(runner, monkeypatch, url): """Test cloning of a Renku repo and existence of required settings.""" import renku.core.storage @@ -58,7 +61,7 @@ def test_renku_clone(runner, monkeypatch, url): @pytest.mark.integration @retry_failed @pytest.mark.parametrize("url", ["https://gitlab.dev.renku.ch/renku-testing/project-9"]) -def test_renku_clone_with_config(tmp_path, url): +def test_clone_with_config(tmp_path, url): """Test cloning of a Renku repo and existence of required settings.""" with chdir(tmp_path): repository, _ = ( @@ -74,7 +77,7 @@ def test_renku_clone_with_config(tmp_path, url): @pytest.mark.integration @retry_failed @pytest.mark.parametrize("url", ["https://gitlab.dev.renku.ch/renku-testing/project-9"]) -def test_renku_clone_checkout_rev(tmp_path, url): +def test_clone_checkout_rev(tmp_path, url): """Test cloning of a repo checking out a rev with static config.""" with chdir(tmp_path): repository, _ = ( @@ -94,7 +97,7 @@ def test_renku_clone_checkout_rev(tmp_path, url): @pytest.mark.integration @retry_failed @pytest.mark.parametrize("rev,detached", [("test-branch", False), ("my-tag", True)]) -def test_renku_clone_checkout_revs(tmp_path, rev, detached): +def test_clone_checkout_revs(tmp_path, rev, detached): """Test cloning of a Renku repo checking out a rev.""" with chdir(tmp_path): repository, _ = ( @@ -113,7 +116,7 @@ def test_renku_clone_checkout_revs(tmp_path, rev, detached): @pytest.mark.integration @pytest.mark.parametrize("path,expected_path", [("", "project-9"), (".", "."), ("new-name", "new-name")]) @retry_failed -def test_renku_clone_uses_project_name(runner, path, expected_path): +def test_clone_uses_project_name(runner, path, expected_path): """Test renku clone uses project name as target-path by default.""" remote = "https://gitlab.dev.renku.ch/renku-testing/project-9" @@ -125,7 +128,7 @@ def test_renku_clone_uses_project_name(runner, path, expected_path): @pytest.mark.integration @retry_failed -def test_renku_clone_private_project_error(runner): +def test_clone_private_project_error(runner): """Test renku clone prints proper error message when a project is private.""" remote = "git@dev.renku.ch:mohammad.alisafaee/test-private-project.git" @@ -135,3 +138,44 @@ def test_renku_clone_private_project_error(runner): assert 0 != result.exit_code assert "Please make sure you have the correct access rights" in result.output assert "and the repository exists." in result.output + + +def test_clone_does_not_change_remote_when_no_credentials(fake_home, mocker, git_repository, runner): + """Test cloning doesn't replace remote URL if no credentials for the hostname exists.""" + + def clone_from(url, *_, **__): + git_repository.run_git_command("remote", "set-url", "origin", url) + return git_repository + + mocker.patch("renku.infrastructure.repository.Repository.clone_from", clone_from) + + result = runner.invoke(cli, ["clone", "https://gitlab.dev.renku.ch/renku-testing/project-9"]) + assert 0 == result.exit_code, format_result_exception(result) + str(result.stderr_bytes) + + with chdir(git_repository.path): + repository = Repository() + + assert 1 == len(repository.remotes) + assert {"origin"} == {remote.name for remote in repository.remotes} + assert repository.remotes["origin"].url.startswith("https://gitlab.dev.renku.ch/renku-testing") + + +def test_clone_changes_remote_when_credentials(fake_home, mocker, git_repository, runner): + """Test cloning replaces remote URL if credentials exists for the hostname.""" + store_token("gitlab.dev.renku.ch", "1234") + + def clone_from(url, *_, **__): + git_repository.run_git_command("remote", "set-url", "origin", url) + return git_repository + + mocker.patch("renku.infrastructure.repository.Repository.clone_from", clone_from) + + result = runner.invoke(cli, ["clone", "https://gitlab.dev.renku.ch/renku-testing/project-9"]) + assert 0 == result.exit_code, format_result_exception(result) + str(result.stderr_bytes) + + with chdir(git_repository.path): + repository = Repository() + + assert 2 == len(repository.remotes) + assert {"origin", "renku-backup-origin"} == {remote.name for remote in repository.remotes} + assert repository.remotes["origin"].url.startswith("https://dev.renku.ch/repos/renku-testing") diff --git a/tests/cli/test_template.py b/tests/cli/test_template.py index b9bc547090..a38348683a 100644 --- a/tests/cli/test_template.py +++ b/tests/cli/test_template.py @@ -350,7 +350,7 @@ def test_template_update_with_parameters_with_defaults(runner, project_with_temp def test_template_set_with_parameters(runner, project_with_template, templates_source, with_injection): - """Test template set doesn't prompts for new template parameters when passed on command line.""" + """Test template set doesn't prompt for new template parameters when passed on command line.""" parameter = TemplateParameter(name="new-parameter", description="", type="", possible_values=[], default=None) templates_source.update(id="dummy", version="2.0.0", parameters=[parameter]) @@ -494,3 +494,25 @@ def test_template_update_with_renames(runner, project_with_template, templates_s assert "2.2.42" == project_context.project.template_metadata.template_ref assert "new-name" == project_context.project.template_metadata.template_id assert not project_with_template.repository.is_dirty() + + +@pytest.mark.parametrize("templates_source", ["renku"], indirect=True) +def test_template_update_with_modified_dockerfile(runner, project_with_template, templates_source, with_injection): + """Test template update only updates Dockerfile's Renku-specific section.""" + dockerfile = project_with_template.path / "Dockerfile" + + write_and_commit_file( + project_with_template.repository, + "Dockerfile", + f"{dockerfile.read_text()}\nRUN local modification outside Renku reserved section", + ) + + templates_source.update(id="dummy", version="2.0.0") + + result = runner.invoke(cli, ["template", "update"]) + + assert result.exit_code == 0, format_result_exception(result) + + assert "RUN local modification outside Renku reserved section" in dockerfile.read_text() + assert "RUN updated specific commands" in dockerfile.read_text() + assert "RUN specific commands" not in dockerfile.read_text() diff --git a/tests/core/test_template.py b/tests/core/test_template.py index b284aa5222..e3039aa336 100644 --- a/tests/core/test_template.py +++ b/tests/core/test_template.py @@ -182,20 +182,36 @@ def test_get_file_actions_for_update(project_with_template, rendered_template_wi identical_file = ".dummy" assert FileAction.IGNORE_IDENTICAL == actions[identical_file] - remotely_modified = "Dockerfile" + remotely_modified = ".gitignore" assert FileAction.OVERWRITE == actions[remotely_modified] + dockerfile = "Dockerfile" + assert FileAction.DOCKERFILE == actions[dockerfile] + + +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 +226,31 @@ def test_update_with_locally_deleted_file(project_with_template, rendered_templa assert FileAction.DELETED == actions["requirements.txt"] +def test_update_with_unsafe_modified_dockerfile(project_with_template, rendered_template_with_update, with_injection): + """Test a locally modified Dockerfile that touches the Renku-specific section 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 + ) + + +def test_update_with_safe_modified_dockerfile(project_with_template, rendered_template_with_update, with_injection): + """Test a locally modified Dockerfile that doesn't touch the Renku-specific section updates the file.""" + dockerfile = project_context.path / "Dockerfile" + dockerfile.write_text(f"{dockerfile.read_text()}\nLocal modification") + + with with_injection(): + actions = get_file_actions( + rendered_template=rendered_template_with_update, template_action=TemplateAction.UPDATE, interactive=False + ) + + assert FileAction.DOCKERFILE == actions["Dockerfile"] + + @pytest.mark.parametrize("delete", [False, True]) def test_update_with_locally_changed_immutable_file( project_with_template, rendered_template_with_update, with_injection, delete diff --git a/tests/fixtures/templates.py b/tests/fixtures/templates.py index d72ab9d218..c74736d500 100644 --- a/tests/fixtures/templates.py +++ b/tests/fixtures/templates.py @@ -132,6 +132,18 @@ def source_template(templates_source_root) -> Template: # Docker content ARG RENKU_VERSION={{ __renku_version__ | default("0.42.0") }} # More content + ######################################################## + # Renku-specific section - DO NOT MODIFY # + ######################################################## + + FROM ${RENKU_BASE_IMAGE} as builder + + RUN specific commands + + ######################################################## + # End Renku-specific section # + ######################################################## + # More content """ ) ) @@ -179,7 +191,11 @@ def update_template_files(templates_source, id, content, parameters): for relative_path in template.get_files(): path = template.path / relative_path - path.write_text(f"{path.read_text()}\n{content}") + + if relative_path == "Dockerfile": + path.write_text(path.read_text().replace("RUN specific commands", "RUN updated specific commands")) + else: + path.write_text(f"{path.read_text()}\n{content}") template.parameters = parameters or [] @@ -308,7 +324,7 @@ def rendered_template_with_update(tmp_path, rendered_template): (updated_template_root / ".gitignore").write_text(".swp\n.idea") (updated_template_root / "requirements.txt").write_text("changed\n") dockerfile = updated_template_root / "Dockerfile" - dockerfile.write_text(f"{dockerfile.read_text()}\n# Updated Dockerfile") + dockerfile.write_text(dockerfile.read_text().replace("RUN specific commands", "RUN updated specific commands")) (updated_template_root / "README.md").write_text("""Updated README: {{ __project_description__ }}\n""") updated_rendered_template = RenderedTemplate( From af22ad474121f8e9284947dcb7dc4c7fa2d562ba Mon Sep 17 00:00:00 2001 From: Mohammad Alisafaee Date: Mon, 24 Apr 2023 17:14:58 +0200 Subject: [PATCH 2/2] address review comments --- renku/command/migrate.py | 2 +- renku/command/view_model/template.py | 2 +- renku/core/migration/migrate.py | 6 +++--- renku/core/template/template.py | 6 +++--- renku/domain_model/template.py | 6 +++--- tests/core/test_template.py | 4 ++-- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/renku/command/migrate.py b/renku/command/migrate.py index 5574184d8e..c04ce1e9e8 100644 --- a/renku/command/migrate.py +++ b/renku/command/migrate.py @@ -133,7 +133,7 @@ def _dockerfile_migration_check(): from renku import __version__ from renku.core.migration.migrate import update_dockerfile - automated_dockerfile_update, newer_renku_available, dockerfile_renku_version = update_dockerfile() + automated_dockerfile_update, newer_renku_available, dockerfile_renku_version = update_dockerfile(check_only=True) return { "automated_dockerfile_update": automated_dockerfile_update, diff --git a/renku/command/view_model/template.py b/renku/command/view_model/template.py index 0a2f53c216..9ad7c9f669 100644 --- a/renku/command/view_model/template.py +++ b/renku/command/view_model/template.py @@ -156,7 +156,7 @@ def from_template( FileAction.KEEP: "Keep", FileAction.OVERWRITE: "Overwrite", FileAction.RECREATE: "Recreate deleted file", - FileAction.DOCKERFILE: "Update", + FileAction.UPDATE_DOCKERFILE: "Update", } file_changes = [ diff --git a/renku/core/migration/migrate.py b/renku/core/migration/migrate.py index f3ed7aab89..3d7b8a9be3 100644 --- a/renku/core/migration/migrate.py +++ b/renku/core/migration/migrate.py @@ -85,7 +85,7 @@ def is_project_unsupported(): def is_docker_update_possible() -> bool: """Check if the Dockerfile can be updated to a new version of renku-python.""" - return update_dockerfile()[0] + return update_dockerfile(check_only=True)[0] @inject.autoparams("project_gateway") @@ -145,7 +145,7 @@ def migrate_project( if not skip_docker_update: try: - docker_updated, _, _ = update_dockerfile(check_only=False) + docker_updated, _, _ = update_dockerfile() except DockerfileUpdateError: raise except Exception as e: @@ -209,7 +209,7 @@ def _update_template() -> bool: return bool(update_template(interactive=False, force=False, dry_run=False)) -def update_dockerfile(check_only=True) -> Tuple[bool, Optional[bool], Optional[str]]: +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__ diff --git a/renku/core/template/template.py b/renku/core/template/template.py index ccc715233f..19f66ea299 100644 --- a/renku/core/template/template.py +++ b/renku/core/template/template.py @@ -74,7 +74,7 @@ class FileAction(IntEnum): KEEP = 6 OVERWRITE = 7 RECREATE = 8 - DOCKERFILE = 9 + UPDATE_DOCKERFILE = 9 def fetch_templates_source(source: Optional[str], reference: Optional[str]) -> TemplatesSource: @@ -144,7 +144,7 @@ def copy_template_metadata_to_project(): FileAction.KEEP: ("", "Keeping"), FileAction.OVERWRITE: ("copy", "Overwriting"), FileAction.RECREATE: ("copy", "Recreating deleted file"), - FileAction.DOCKERFILE: ("dockerfile", "Updating"), + FileAction.UPDATE_DOCKERFILE: ("dockerfile", "Updating"), } for relative_path, action in get_sorted_actions(actions=actions).items(): @@ -259,7 +259,7 @@ def get_action_for_update( if is_dockerfile_updated_by_user(): raise errors.TemplateUpdateError("Can't update template as Dockerfile was locally changed.") else: - return FileAction.DOCKERFILE + 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 diff --git a/renku/domain_model/template.py b/renku/domain_model/template.py index 8f02d8ab0b..3cf8e9a057 100644 --- a/renku/domain_model/template.py +++ b/renku/domain_model/template.py @@ -603,7 +603,7 @@ def find_renku_section(lines: List[str]) -> Tuple[int, int]: return start, end -def get_renku_section_from_dockerfile(content: str) -> str: +def get_renku_section_from_dockerfile(content: str) -> Optional[str]: """Return the Renku-specific section of the Dockerfile or the whole Dockerfile if it doesn't exist.""" lines = [line.rstrip() for line in content.splitlines()] start, end = find_renku_section(lines) @@ -613,7 +613,7 @@ def get_renku_section_from_dockerfile(content: str) -> str: lines = [line for line in lines if line] # NOTE: Remove empty lines return "\n".join(lines) else: - return content + return None def calculate_dockerfile_checksum( @@ -630,7 +630,7 @@ def calculate_dockerfile_checksum( 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) + renku_section = get_renku_section_from_dockerfile(content) or content return hash_string(renku_section) diff --git a/tests/core/test_template.py b/tests/core/test_template.py index e3039aa336..c1f1a5441b 100644 --- a/tests/core/test_template.py +++ b/tests/core/test_template.py @@ -185,7 +185,7 @@ def test_get_file_actions_for_update(project_with_template, rendered_template_wi remotely_modified = ".gitignore" assert FileAction.OVERWRITE == actions[remotely_modified] dockerfile = "Dockerfile" - assert FileAction.DOCKERFILE == actions[dockerfile] + assert FileAction.UPDATE_DOCKERFILE == actions[dockerfile] def test_template_set_with_locally_modified_dockerfile( @@ -248,7 +248,7 @@ def test_update_with_safe_modified_dockerfile(project_with_template, rendered_te rendered_template=rendered_template_with_update, template_action=TemplateAction.UPDATE, interactive=False ) - assert FileAction.DOCKERFILE == actions["Dockerfile"] + assert FileAction.UPDATE_DOCKERFILE == actions["Dockerfile"] @pytest.mark.parametrize("delete", [False, True])