From d97055e2ba353e595958e87f7a570f84d3091345 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 14 Nov 2018 09:15:58 +0200 Subject: [PATCH] Code review fixes --- data/model/oci/manifest.py | 13 ++++++++++--- endpoints/v2/test/test_manifest_cornercases.py | 6 +++--- image/docker/interfaces.py | 15 +++++++++------ image/docker/schema1.py | 2 +- image/docker/schema2/test/test_manifest.py | 2 -- workers/manifestbackfillworker.py | 7 +++++++ 6 files changed, 30 insertions(+), 15 deletions(-) diff --git a/data/model/oci/manifest.py b/data/model/oci/manifest.py index 89a8e2c82..3d2d217a5 100644 --- a/data/model/oci/manifest.py +++ b/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() diff --git a/endpoints/v2/test/test_manifest_cornercases.py b/endpoints/v2/test/test_manifest_cornercases.py index a2deee92f..e4570da65 100644 --- a/endpoints/v2/test/test_manifest_cornercases.py +++ b/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() diff --git a/image/docker/interfaces.py b/image/docker/interfaces.py index d724c6043..e8cbd8dd9 100644 --- a/image/docker/interfaces.py +++ b/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 diff --git a/image/docker/schema1.py b/image/docker/schema1.py index ca58d8724..11ace16e8 100644 --- a/image/docker/schema1.py +++ b/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): diff --git a/image/docker/schema2/test/test_manifest.py b/image/docker/schema2/test/test_manifest.py index afc918054..3961be53f 100644 --- a/image/docker/schema2/test/test_manifest.py +++ b/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 diff --git a/workers/manifestbackfillworker.py b/workers/manifestbackfillworker.py index 3dd4d2153..f1260305b 100644 --- a/workers/manifestbackfillworker.py +++ b/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):