Code review fixes

data/model/oci/manifest.py

@@ -95,8 +95,13 @@ def _create_manifest(repository_id, manifest_interface_instance, storage):
         logger.exception('Could not load manifest labels for child manifest')
         return None
 
-      # Get/create the child manifest in the database.
+      # NOTE: Content type restrictions in manifest lists ensure that the child manifests
+      # must be image manifests, as opposed to lists themselves. We put this check here to
+      # be extra careful in ensuring we don't create empty manifests. If this reality changes,
+      # should remove this check.
       assert list(child_manifest.layers)
+
+      # Get/create the child manifest in the database.
       child_manifest_info = get_or_create_manifest(repository_id, child_manifest, storage)
       if child_manifest_info is None:
         logger.error('Could not get/create child manifest')
@@ -166,8 +171,10 @@ def _create_manifest(repository_id, manifest_interface_instance, storage):
       media_type = 'application/json' if is_json(value) else 'text/plain'
       create_manifest_label(manifest, key, value, 'manifest', media_type)
 
-  # Return the dictionary of labels to apply. We only return those labels either defined on
-  # the manifest or shared amongst all the child manifest.
+  # Return the dictionary of labels to apply (i.e. those labels that cause an action to be taken
+  # on the manifest or its resulting tags). We only return those labels either defined on
+  # the manifest or shared amongst all the child manifests. We intersect amongst all child manifests
+  # to ensure that any action performed is defined in all manifests.
   labels_to_apply = labels or {}
   if child_manifest_label_dicts:
     labels_to_apply = child_manifest_label_dicts[0].viewitems()

endpoints/v2/test/test_manifest_cornercases.py

@@ -65,7 +65,7 @@ def test_missing_link(initialized_db):
                       .add_layer(first_blob_sha, '{"id": "first"}')
                       .build(docker_v2_signing_key))
 
-    _write_manifest(ADMIN_ACCESS_USER, REPO, first_manifest)
+    _write_manifest(ADMIN_ACCESS_USER, REPO, FIRST_TAG, first_manifest)
 
     # Delete all temp tags and perform GC.
     _perform_cleanup()
@@ -88,7 +88,7 @@ def test_missing_link(initialized_db):
                         .add_layer(second_blob_sha, '{"id": "first"}')
                         .build(docker_v2_signing_key))
 
-    _write_manifest(ADMIN_ACCESS_USER, REPO, second_manifest)
+    _write_manifest(ADMIN_ACCESS_USER, REPO, SECOND_TAG, second_manifest)
 
     # Delete all temp tags and perform GC.
     _perform_cleanup()
@@ -117,7 +117,7 @@ def test_missing_link(initialized_db):
                       .add_layer(fourth_blob_sha, '{"id": "first"}')  # Note the change in BLOB from the second manifest.
                       .build(docker_v2_signing_key))
 
-    _write_manifest(ADMIN_ACCESS_USER, REPO, third_manifest)
+    _write_manifest(ADMIN_ACCESS_USER, REPO, THIRD_TAG, third_manifest)
 
     # Delete all temp tags and perform GC.
     _perform_cleanup()

image/docker/interfaces.py

@@ -30,12 +30,14 @@ class ManifestInterface(object):
 
   @abstractproperty
   def layers(self):
-    """ Returns the layers of this manifest, from base to leaf or None if none. """
+    """ Returns the layers of this manifest, from base to leaf or None if this kind of manifest
+        does not support layers. """
     pass
 
   @abstractproperty
   def leaf_layer_v1_image_id(self):
-    """ Returns the Docker V1 image ID for the leaf (top) layer, if any, or None if none. """
+    """ Returns the Docker V1 image ID for the leaf (top) layer, if any, or None if
+        not applicable. """
     pass
 
   @abstractproperty
@@ -53,14 +55,15 @@ class ManifestInterface(object):
 
   @abstractmethod
   def child_manifests(self, lookup_manifest_fn):
-    """ Returns an iterator of all manifests that live under this manifest, if any or None if none.
-        The lookup_manifest_fn is a function that, when given a blob content SHA, returns the
-        contents of that blob in storage if any or None if none.
+    """ Returns an iterator of all manifests that live under this manifest, if any or None if not
+        applicable. The lookup_manifest_fn is a function that, when given a blob content SHA,
+        returns the contents of that blob in storage if any or None if none.
     """
 
   @abstractmethod
   def get_manifest_labels(self, lookup_config_fn):
-    """ Returns a dictionary of all the labels defined inside this manifest or None if none. """
+    """ Returns a dictionary of all the labels defined inside this manifest or None if this kind
+        of manifest does not support labels. """
     pass
 
   @abstractmethod

image/docker/schema1.py

@@ -317,7 +317,7 @@ class DockerSchema1Manifest(ManifestInterface):
                                            self._architecture)
     for layer in reversed(self.layers):
       builder.add_layer(str(layer.digest), layer.raw_v1_metadata)
- 
+
     return builder.build()
 
   def _generate_layers(self):

image/docker/schema2/test/test_manifest.py

@@ -73,8 +73,6 @@ def test_valid_manifest():
   assert manifest.leaf_layer.compressed_size == 73109
 
   blob_digests = list(manifest.blob_digests)
-  assert len(blob_digests) == len(manifest.layers) + 1
-
   expected = [str(layer.digest) for layer in manifest.layers] + [manifest.config.digest]
   assert blob_digests == expected
 

workers/manifestbackfillworker.py

@@ -75,6 +75,13 @@ class BrokenManifest(ManifestInterface):
   def generate_legacy_layers(self, images_map, lookup_config_fn):
     return None
 
+  def get_v1_compatible_manifest(self, namespace_name, repo_name, tag_name, lookup_fn):
+    return self
+    
+  @property
+  def schema_version():
+    return 1
+
 
 class ManifestBackfillWorker(Worker):
   def __init__(self):