fix(core): migrate Dockerfile after metadata (#3690)

* fix(core): migrate Dockerfile after metadata

renku/core/migration/migrate.py

@@ -123,6 +123,42 @@ def migrate_project(
     if not is_renku_project():
         return False, template_updated, docker_updated
 
+    n_migrations_executed = 0
+
+    if not skip_migrations:
+        project_version = project_version or get_project_version()
+
+        migration_context = MigrationContext(
+            strict=strict, type=migration_type, preserve_identifiers=preserve_identifiers
+        )
+
+        version = 1
+        for version, path in get_migrations():
+            if max_version and version > max_version:
+                break
+            if version > project_version:
+                module = importlib.import_module(path)
+                module_name = module.__name__.split(".")[-1]
+                communication.echo(f"Applying migration {module_name}...")
+                try:
+                    module.migrate(migration_context)
+                except (Exception, BaseException) as e:
+                    raise MigrationError("Couldn't execute migration") from e
+                n_migrations_executed += 1
+
+        if not is_using_temporary_datasets_path():
+            if n_migrations_executed > 0:
+                project_context.project.version = str(version)
+                project_gateway.update_project(project_context.project)
+
+                communication.echo(f"Successfully applied {n_migrations_executed} migrations.")
+
+            _remove_untracked_renku_files(metadata_path=project_context.metadata_path)
+
+        # we might not have been able to tell if a docker update is possible due to outstanding migrations.
+        # so we need to check again here.
+        skip_docker_update |= not is_docker_update_possible()
+
     try:
         project = project_context.project
     except ValueError:
@@ -155,36 +191,6 @@ def migrate_project(
         except Exception as e:
             raise DockerfileUpdateError("Couldn't update renku version in Dockerfile.") from e
 
-    if skip_migrations:
-        return False, template_updated, docker_updated
-
-    project_version = project_version or get_project_version()
-    n_migrations_executed = 0
-
-    migration_context = MigrationContext(strict=strict, type=migration_type, preserve_identifiers=preserve_identifiers)
-
-    version = 1
-    for version, path in get_migrations():
-        if max_version and version > max_version:
-            break
-        if version > project_version:
-            module = importlib.import_module(path)
-            module_name = module.__name__.split(".")[-1]
-            communication.echo(f"Applying migration {module_name}...")
-            try:
-                module.migrate(migration_context)
-            except (Exception, BaseException) as e:
-                raise MigrationError("Couldn't execute migration") from e
-            n_migrations_executed += 1
-    if not is_using_temporary_datasets_path():
-        if n_migrations_executed > 0:
-            project_context.project.version = str(version)
-            project_gateway.update_project(project_context.project)
-
-            communication.echo(f"Successfully applied {n_migrations_executed} migrations.")
-
-        _remove_untracked_renku_files(metadata_path=project_context.metadata_path)
-
     return n_migrations_executed != 0, template_updated, docker_updated
 
 

renku/ui/cli/migrate.py

@@ -98,6 +98,7 @@ def migrate(check, skip_template_update, skip_docker_update, strict, preserve_id
 
     template_update_possible = status & TEMPLATE_UPDATE_POSSIBLE and status & AUTOMATED_TEMPLATE_UPDATE_SUPPORTED
     docker_update_possible = status & DOCKERFILE_UPDATE_POSSIBLE
+    migration_required = status & MIGRATION_REQUIRED
 
     if check:
         if template_update_possible:
@@ -113,7 +114,7 @@ def migrate(check, skip_template_update, skip_docker_update, strict, preserve_id
                 + "using 'renku migrate'."
             )
 
-        if status & MIGRATION_REQUIRED:
+        if migration_required:
             raise MigrationRequired
 
     if status & UNSUPPORTED_PROJECT:
@@ -125,7 +126,9 @@ def migrate(check, skip_template_update, skip_docker_update, strict, preserve_id
     if check:
         return
 
-    skip_docker_update = skip_docker_update or not docker_update_possible
+    # In case where a migration is required, we can't tell if a dockerupdate is possible, as the metadata for deciding
+    # might not be there yet. We'll check this again after the migration
+    skip_docker_update = skip_docker_update or (not migration_required and not docker_update_possible)
     skip_template_update = skip_template_update or not template_update_possible
 
     communicator = ClickCallback()

renku/ui/service/controllers/cache_migrate_project.py

@@ -33,6 +33,7 @@ def execute_migration(
     from renku.command.migrate import (
         AUTOMATED_TEMPLATE_UPDATE_SUPPORTED,
         DOCKERFILE_UPDATE_POSSIBLE,
+        MIGRATION_REQUIRED,
         TEMPLATE_UPDATE_POSSIBLE,
         check_project,
         migrate_project_command,
@@ -47,8 +48,9 @@ def execute_migration(
 
         template_update_possible = status & TEMPLATE_UPDATE_POSSIBLE and status & AUTOMATED_TEMPLATE_UPDATE_SUPPORTED
         docker_update_possible = status & DOCKERFILE_UPDATE_POSSIBLE
+        migration_required = status & MIGRATION_REQUIRED
 
-        skip_docker_update = skip_docker_update or not docker_update_possible
+        skip_docker_update = skip_docker_update or (not migration_required and not docker_update_possible)
         skip_template_update = skip_template_update or not template_update_possible
 
         result = (

tests/cli/test_migrate.py

@@ -53,6 +53,27 @@ def test_migrate_project(isolated_runner, old_project, with_injection):
         assert project_context.project.name
 
 
+@pytest.mark.migration
+@pytest.mark.parametrize(
+    "old_project",
+    [
+        "v9-migration-docker-version-change.git",
+        "v9-migration-docker-mult-changes.git",
+    ],
+    indirect=["old_project"],
+)
+def test_migrate_old_project_with_docker_change(isolated_runner, old_project, with_injection):
+    """Test migrating projects with changes to the Dockerfile."""
+    result = isolated_runner.invoke(cli, ["migrate", "--strict"])
+    assert 0 == result.exit_code, format_result_exception(result)
+    assert not old_project.repository.is_dirty()
+    assert "Updated dockerfile" in result.output
+
+    with project_context.with_path(old_project.path), with_injection():
+        assert project_context.project
+        assert project_context.project.name
+
+
 @pytest.mark.migration
 @pytest.mark.serial
 def test_migration_check(isolated_runner, project):

tests/service/fixtures/service_integration.py

@@ -25,7 +25,7 @@
 
 from renku.core import errors
 from renku.infrastructure.repository import Repository
-from tests.utils import format_result_exception, modified_environ
+from tests.utils import clone_compressed_repository, format_result_exception, modified_environ
 
 
 @contextlib.contextmanager
@@ -272,6 +272,57 @@ def _parse(href):
             pass
 
 
+@pytest.fixture
+def old_local_remote_project(request, svc_client, tmp_path, mock_redis, identity_headers, real_sync):
+    """Fixture for testing service with project tarfiles containing old projects."""
+    from renku.domain_model import git
+    from renku.ui.service.cache import cache as redis_cache
+    from renku.ui.service.gateways.repository_cache import LocalRepositoryCache
+    from renku.ui.service.serializers.headers import RequiredIdentityHeaders
+
+    name = request.param
+    remote_repo_path = tmp_path / name
+    remote_repo = clone_compressed_repository(base_path=tmp_path, name=name)
+    remote_repo_path = remote_repo_path / "repository"
+    remote_repo_checkout_path = tmp_path / "remote_repo_checkout"
+    remote_repo_checkout_path.mkdir()
+
+    remote_repo_checkout = Repository.clone_from(url=remote_repo_path, path=remote_repo_checkout_path)
+
+    # NOTE: Mock GitURL parsing for local URL
+    def _parse(href):
+        return git.GitURL(href=href, regex="", owner="dummy", name="project", slug="project", path=remote_repo_path)
+
+    original_giturl_parse = git.GitURL.parse
+    git.GitURL.parse = _parse
+
+    home = tmp_path / "user_home"
+    home.mkdir()
+
+    user_data = RequiredIdentityHeaders().load(identity_headers)
+    user = redis_cache.ensure_user(user_data)
+    remote_url = f"file://{remote_repo_path}"
+
+    project = LocalRepositoryCache().get(redis_cache, remote_url, branch=None, user=user, shallow=False)
+
+    project_id = project.project_id
+
+    try:
+        yield svc_client, identity_headers, project_id, remote_repo, remote_repo_checkout, remote_url
+    finally:
+        git.GitURL.parse = original_giturl_parse
+
+        try:
+            shutil.rmtree(remote_repo_path)
+        except OSError:
+            pass
+
+        try:
+            shutil.rmtree(remote_repo_checkout_path)
+        except OSError:
+            pass
+
+
 @pytest.fixture
 def quick_cache_synchronization(mocker):
     """Forces cache to synchronize on every request."""

tests/service/views/test_cache_views.py

@@ -704,6 +704,25 @@ def test_execute_migrations(svc_client_setup):
     assert not response.json["result"]["errors"]
 
 
+@pytest.mark.service
+@pytest.mark.parametrize(
+    "old_local_remote_project",
+    [
+        "v9-migration-docker-version-change.git",
+        "v9-migration-docker-mult-changes.git",
+    ],
+    indirect=["old_local_remote_project"],
+)
+def test_migrate_old_project(old_local_remote_project):
+    """Test migrating old projects with docker file changes."""
+    svc_client, identity_headers, project_id, remote_repo, remote_repo_checkout, remote_url = old_local_remote_project
+
+    response = svc_client.post("/cache.migrate", data=json.dumps(dict(git_url=remote_url)), headers=identity_headers)
+    assert response
+    assert 200 == response.status_code
+    assert response.json.get("result", {}).get("docker_migrated", False)
+
+
 @pytest.mark.service
 @pytest.mark.integration
 def test_execute_migrations_job(svc_client_setup):