fix: migration of old datasets (#639)

Author: jsam

conftest.py

@@ -251,7 +251,24 @@ def data_repository(directory_tree):
 
 
 @pytest.fixture(
-    params=['test-renku-v0.3.0.git', 'old-datasets-v0.3.0.git'],
+    params=[
+        {
+            'name': 'old-datasets-v0.3.0.git',
+            'exit_code': 1
+        },
+        {
+            'name': 'old-datasets-v0.5.0.git',
+            'exit_code': 1
+        },
+        {
+            'name': 'old-datasets-v0.5.1.git',
+            'exit_code': 0
+        },
+        {
+            'name': 'test-renku-v0.3.0.git',
+            'exit_code': 1
+        },
+    ],
     scope='module',
 )
 def old_bare_repository(request, tmpdir_factory):
@@ -261,13 +278,19 @@ def old_bare_repository(request, tmpdir_factory):
 
     compressed_repo_path = Path(
         __file__
-    ).parent / 'tests' / 'fixtures' / '{0}.tar.gz'.format(request.param)
-    working_dir_path = tmpdir_factory.mktemp(request.param)
+    ).parent / 'tests' / 'fixtures' / '{0}.tar.gz'.format(
+        request.param['name']
+    )
+
+    working_dir_path = tmpdir_factory.mktemp(request.param['name'])
 
     with tarfile.open(str(compressed_repo_path), 'r') as fixture:
         fixture.extractall(working_dir_path.strpath)
 
-    yield working_dir_path / request.param
+    yield {
+        'path': working_dir_path / request.param['name'],
+        'exit_code': request.param['exit_code']
+    }
 
     shutil.rmtree(working_dir_path.strpath)
 
@@ -279,21 +302,29 @@ def old_repository(tmpdir_factory, old_bare_repository):
     from git import Repo
 
     repo_path = tmpdir_factory.mktemp('repo')
-    yield Repo(old_bare_repository.strpath).clone(repo_path.strpath)
+    yield {
+        'repo':
+            Repo(old_bare_repository['path'].strpath).clone(repo_path.strpath),
+        'exit_code': old_bare_repository['exit_code']
+    }
     shutil.rmtree(repo_path.strpath)
 
 
 @pytest.fixture
 def old_project(old_repository):
     """Create a test project."""
-    repo = old_repository
-    repository = repo.working_dir
+    repo = old_repository['repo']
+    repository_path = repo.working_dir
 
     commit = repo.head.commit
 
-    os.chdir(repository)
-    yield repository
-    os.chdir(repository)
+    os.chdir(repository_path)
+    yield {
+        'repo': repo,
+        'path': repository_path,
+        'exit_code': old_repository['exit_code']
+    }
+    os.chdir(repository_path)
     repo.head.reset(commit, index=True, working_tree=True)
     # remove any extra non-tracked files (.pyc, etc)
     repo.git.clean('-xdff')

renku/api/_git.py

@@ -34,6 +34,7 @@
 
 from renku import errors
 from renku._compat import Path
+from renku.errors import NothingToCommit
 
 COMMIT_DIFF_STRATEGY = 'DIFF'
 STARTED_AT = int(time.time() * 1e3)
@@ -229,7 +230,7 @@ def ensure_unstaged(self, path):
             pass
 
     @contextmanager
-    def commit(self, author_date=None, commit_only=None):
+    def commit(self, author_date=None, commit_only=None, allow_empty=True):
         """Automatic commit."""
         from git import Actor
         from renku.version import __version__, version_url
@@ -286,6 +287,9 @@ def commit(self, author_date=None, commit_only=None):
         if not commit_only:
             self.repo.git.add('--all')
 
+        if not allow_empty and not self.repo.index.diff('HEAD'):
+            raise NothingToCommit()
+
         argv = [os.path.basename(sys.argv[0])] + sys.argv[1:]
 
         # Ignore pre-commit hooks since we have already done everything.
@@ -303,7 +307,8 @@ def transaction(
         up_to_date=False,
         commit=True,
         commit_only=None,
-        ignore_std_streams=False
+        ignore_std_streams=False,
+        allow_empty=True,
     ):
         """Perform Git checks and operations."""
         if clean:
@@ -316,7 +321,7 @@ def transaction(
             pass
 
         if commit:
-            with self.commit(commit_only=commit_only):
+            with self.commit(commit_only=commit_only, allow_empty=allow_empty):
                 yield self
         else:
             yield self

renku/api/repository.py

@@ -16,7 +16,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Client for handling a local repository."""
-
+import subprocess
 import uuid
 from collections import defaultdict
 from contextlib import contextmanager
@@ -99,11 +99,13 @@ def __attrs_post_init__(self):
 
         # initialize submodules
         if self.repo:
-            check_output([
-                'git', 'submodule', 'update', '--init', '--recursive'
-            ],
-                         cwd=str(self.path))
-        # TODO except
+            try:
+                check_output([
+                    'git', 'submodule', 'update', '--init', '--recursive'
+                ],
+                             cwd=str(self.path))
+            except subprocess.CalledProcessError:
+                pass
 
     @property
     def lock(self):
@@ -251,42 +253,49 @@ def subclients(self, parent_commit):
                 Submodule.iter_items(self.repo, parent_commit=parent_commit)
             ]
         except (RuntimeError, ValueError):
-            # There are no submodules assiciated with the given commit.
+            # There are no submodules associated with the given commit.
             submodules = []
 
-        return self._subclients.setdefault(
-            parent_commit, {
-                submodule: self.__class__(
-                    path=(self.path / submodule.path).resolve(),
+        subclients = {}
+        for submodule in submodules:
+            subpath = (self.path / submodule.path).resolve()
+            is_renku = subpath / Path(self.renku_home)
+
+            if subpath.exists() and is_renku.exists():
+                subclients[submodule] = self.__class__(
+                    path=subpath,
                     parent=(self, submodule),
                 )
-                for submodule in submodules
-            }
-        )
+
+        return subclients
 
     def resolve_in_submodules(self, commit, path):
         """Resolve filename in submodules."""
         original_path = self.path / path
-        if original_path.is_symlink() or str(path
-                                             ).startswith('.renku/vendors'):
+        in_vendor = str(path).startswith('.renku/vendors')
+
+        if original_path.is_symlink() or in_vendor:
             original_path = original_path.resolve()
+
             for submodule, subclient in self.subclients(commit).items():
-                try:
-                    subpath = original_path.relative_to(subclient.path)
-                    return (
-                        subclient,
-                        subclient.find_previous_commit(
-                            subpath, revision=submodule.hexsha
-                        ),
-                        subpath,
-                    )
-                except ValueError:
-                    pass
+                if (Path(submodule.path) / Path('.git')).exists():
+
+                    try:
+                        subpath = original_path.relative_to(subclient.path)
+                        return (
+                            subclient,
+                            subclient.find_previous_commit(
+                                subpath, revision=submodule.hexsha
+                            ),
+                            subpath,
+                        )
+                    except ValueError:
+                        pass
 
         return self, commit, path
 
     @contextmanager
-    def with_metadata(self):
+    def with_metadata(self, read_only=False):
         """Yield an editable metadata object."""
         from renku.models.projects import Project
 
@@ -299,7 +308,8 @@ def with_metadata(self):
 
         yield metadata
 
-        metadata.to_yaml()
+        if not read_only:
+            metadata.to_yaml()
 
     @contextmanager
     def with_workflow_storage(self):

renku/cli/_checks/__init__.py

@@ -17,11 +17,11 @@
 # limitations under the License.
 """Define repository checks for :program:`renku doctor`."""
 
-from .files_in_datasets import check_missing_files
-from .location_datasets import check_dataset_metadata
+from .migrate_datasets import (check_dataset_metadata, check_missing_files)
 from .references import check_missing_references
 
-# Checks will be executed in the order as they are listed in __all__
+# Checks will be executed in the order as they are listed in __all__.
+# They are mostly used in ``doctor`` command to inspect broken things.
 __all__ = (
     'check_dataset_metadata',
     'check_missing_files',