Merge pull request #2907 from coreos-inc/joseph.schorr/QS-55/fix-tag-removal
Fix bug in listing owned tags
This commit is contained in:
		
						commit
						6bc39a0b8c
					
				
					 5 changed files with 27 additions and 145 deletions
				
			
		|  | @ -172,14 +172,11 @@ class RepositoryTagImages(RepositoryParamResource): | |||
|     if tag_image is None: | ||||
|       raise NotFound() | ||||
| 
 | ||||
|     # Find all the parent images for the tag. | ||||
|     parent_images = model.get_parent_images(namespace, repository, tag_image.docker_image_id) | ||||
|     image_map = {str(tag_image.docker_image_id): tag_image} | ||||
| 
 | ||||
|     for image in parent_images: | ||||
|       image_map[str(image.docker_image_id)] = image | ||||
| 
 | ||||
|     image_map_all = dict(image_map) | ||||
|     all_images = [tag_image] + list(parent_images) | ||||
|     image_map = {image.docker_image_id: image for image in all_images} | ||||
|     skip_set = set() | ||||
| 
 | ||||
|     # Filter the images returned to those not found in the ancestry of any of the other tags in | ||||
|     # the repository. | ||||
|  | @ -189,18 +186,13 @@ class RepositoryTagImages(RepositoryParamResource): | |||
|         if current_tag.name == tag: | ||||
|           continue | ||||
| 
 | ||||
|         # Remove the tag's image ID. | ||||
|         tag_image_id = str(current_tag.docker_image_id) | ||||
|         image_map.pop(tag_image_id, None) | ||||
| 
 | ||||
|         # Remove any ancestors: | ||||
|         for ancestor_id in current_tag.image.ancestors.split('/'): | ||||
|           image_map.pop(ancestor_id, None) | ||||
|         skip_set.add(current_tag.image.ancestor_id) | ||||
|         skip_set = skip_set | set(current_tag.image.ancestor_id_list) | ||||
| 
 | ||||
|     return { | ||||
|       'images': [ | ||||
|         image.to_dict(image_map_all) for image in all_images | ||||
|         if not parsed_args['owned'] or (str(image.docker_image_id) in image_map) | ||||
|         image.to_dict(image_map) for image in all_images | ||||
|         if not parsed_args['owned'] or (image.ancestor_id not in skip_set) | ||||
|       ] | ||||
|     } | ||||
| 
 | ||||
|  |  | |||
|  | @ -61,7 +61,7 @@ class Repository(namedtuple('Repository', ['namespace_name', 'repository_name']) | |||
| class Image( | ||||
|   namedtuple('Image', [ | ||||
|     'docker_image_id', 'created', 'comment', 'command', 'storage_image_size', | ||||
|     'storage_uploading', 'ancestor_id_list' | ||||
|     'storage_uploading', 'ancestor_id_list', 'ancestor_id' | ||||
|   ])): | ||||
|   """ | ||||
|   Image | ||||
|  | @ -72,6 +72,7 @@ class Image( | |||
|   :type storage_image_size: int | ||||
|   :type storage_uploading: boolean | ||||
|   :type ancestor_id_list: [int] | ||||
|   :type ancestor_id: int | ||||
|   """ | ||||
| 
 | ||||
|   def to_dict(self, image_map, include_ancestors=True): | ||||
|  |  | |||
|  | @ -120,11 +120,12 @@ def convert_image(database_image): | |||
|                comment=database_image.comment, command=database_image.command, | ||||
|                storage_image_size=database_image.storage.image_size, | ||||
|                storage_uploading=database_image.storage.uploading, | ||||
|                ancestor_id_list=database_image.ancestor_id_list()) | ||||
|                ancestor_id_list=database_image.ancestor_id_list(), | ||||
|                ancestor_id=database_image.id) | ||||
| 
 | ||||
| 
 | ||||
| def convert_tag(tag, manifest_list=None): | ||||
|   return Tag(name=tag.name, image=tag.image, reversion=tag.reversion, | ||||
|   return Tag(name=tag.name, image=convert_image(tag.image), reversion=tag.reversion, | ||||
|              lifetime_start_ts=tag.lifetime_start_ts, lifetime_end_ts=tag.lifetime_end_ts, | ||||
|              manifest_list=manifest_list, docker_image_id=tag.image.docker_image_id) | ||||
| 
 | ||||
|  |  | |||
|  | @ -8,7 +8,7 @@ from data.model import DataModelException | |||
| from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag | ||||
| from endpoints.api.test.shared import conduct_api_call | ||||
| from endpoints.test.shared import client_with_identity | ||||
| from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags | ||||
| from endpoints.api.tag import RepositoryTag, RestoreTag, ListRepositoryTags, RepositoryTagImages | ||||
| 
 | ||||
| from features import FeatureNameValue | ||||
| 
 | ||||
|  | @ -281,3 +281,17 @@ def test_restore_tag(test_manifest, test_tag, manifest_generated, expected_statu | |||
| 
 | ||||
|   if manifest_generated: | ||||
|     generate_manifest.assert_called_with('devtable', 'simple', test_tag) | ||||
| 
 | ||||
| @pytest.mark.parametrize('repository, tag, owned, expect_images', [ | ||||
|   ('devtable/simple', 'prod', False, True), | ||||
|   ('devtable/simple', 'prod', True, False), | ||||
|   ('devtable/simple', 'latest', False, True), | ||||
|   ('devtable/simple', 'latest', True, False), | ||||
| 
 | ||||
|   ('devtable/complex', 'prod', False, True), | ||||
|   ('devtable/complex', 'prod', True, True), | ||||
| ]) | ||||
| def test_list_tag_images(repository, tag, owned, expect_images, authd_client): | ||||
|   params = {'repository': repository, 'tag': tag, 'owned': owned} | ||||
|   result = conduct_api_call(authd_client, RepositoryTagImages, 'get', params, None, 200).json | ||||
|   assert bool(result['images']) == expect_images | ||||
|  |  | |||
|  | @ -34,78 +34,6 @@ def mock_out_get_repository(monkeypatch, namespace_name, repository_name): | |||
|   monkeypatch.setattr(model.repository, 'get_repository', return_function) | ||||
| 
 | ||||
| 
 | ||||
| def create_mock_tag(name, reversion, lifetime_start_ts, lifetime_end_ts, mock_id, docker_image_id, | ||||
|                     manifest_list): | ||||
|   tag_mock = Mock() | ||||
|   tag_mock.name = name | ||||
|   image_mock = Mock() | ||||
|   image_mock.docker_image_id = docker_image_id | ||||
|   tag_mock.image = image_mock | ||||
|   tag_mock.reversion = reversion | ||||
|   tag_mock.lifetime_start_ts = lifetime_start_ts | ||||
|   tag_mock.lifetime_end_ts = lifetime_end_ts | ||||
|   tag_mock.id = mock_id | ||||
|   tag_mock.manifest_list = manifest_list | ||||
|   tag = Tag(name=name, reversion=reversion, image=image_mock, docker_image_id=docker_image_id, | ||||
|             lifetime_start_ts=lifetime_start_ts, lifetime_end_ts=lifetime_end_ts, | ||||
|             manifest_list=manifest_list) | ||||
|   return tag_mock, tag | ||||
| 
 | ||||
| 
 | ||||
| first_mock, first_tag = create_mock_tag('tag1', 'rev1', 'start1', 'end1', 'id1', | ||||
|                                         'docker_image_id1', []) | ||||
| second_mock, second_tag = create_mock_tag('tag2', 'rev2', 'start2', 'end2', 'id2', | ||||
|                                           'docker_image_id2', ['manifest']) | ||||
| 
 | ||||
| 
 | ||||
| def mock_out_list_repository_tag_history(monkeypatch, namespace_name, repository_name, page, size, | ||||
|                                          specific_tag): | ||||
|   def list_empty_tag_history(repository, page, size, specific_tag): | ||||
|     return [], {}, False | ||||
| 
 | ||||
|   def list_filled_tag_history(repository, page, size, specific_tag): | ||||
|     tags = [first_mock, second_mock] | ||||
|     return tags, { | ||||
|       first_mock.id: first_mock.manifest_list, | ||||
|       second_mock.id: second_mock.manifest_list | ||||
|     }, len(tags) > size | ||||
| 
 | ||||
|   def list_only_second_tag(repository, page, size, specific_tag): | ||||
|     tags = [second_mock] | ||||
|     return tags, {second_mock.id: second_mock.manifest_list}, len(tags) > size | ||||
| 
 | ||||
|   if namespace_name == EMPTY_NAMESPACE or repository_name == EMPTY_REPOSITORY: | ||||
|     return_function = list_empty_tag_history | ||||
|   else: | ||||
|     if specific_tag == 'tag2': | ||||
|       return_function = list_only_second_tag | ||||
|     else: | ||||
|       return_function = list_filled_tag_history | ||||
| 
 | ||||
|   monkeypatch.setattr(model.tag, 'list_repository_tag_history', return_function) | ||||
| 
 | ||||
| 
 | ||||
| @pytest.mark.parametrize( | ||||
|   'expected, namespace_name,        repository_name,       page,  size, specific_tag', [ | ||||
|     (None, BAD_NAMESPACE_NAME, 'repository_name', 1, 100, None), | ||||
|     (None, 'namespace_name', BAD_REPOSITORY_NAME, 1, 100, None), | ||||
|     (RepositoryTagHistory(tags=[], more=False), EMPTY_NAMESPACE, EMPTY_REPOSITORY, 1, 100, None), | ||||
|     (RepositoryTagHistory(tags=[first_tag, second_tag], more=False), 'namespace', 'repository', 1, | ||||
|      100, None), | ||||
|     (RepositoryTagHistory(tags=[first_tag, second_tag], more=True), 'namespace', 'repository', 1, | ||||
|      1, None), | ||||
|     (RepositoryTagHistory(tags=[second_tag], more=False), 'namespace', 'repository', 1, 100, | ||||
|      'tag2'), | ||||
|   ]) | ||||
| def test_list_repository_tag_history(expected, namespace_name, repository_name, page, size, | ||||
|                                      specific_tag, get_monkeypatch): | ||||
|   mock_out_get_repository(get_monkeypatch, namespace_name, repository_name) | ||||
|   mock_out_list_repository_tag_history(get_monkeypatch, namespace_name, repository_name, page, | ||||
|                                        size, specific_tag) | ||||
|   assert pre_oci_model.list_repository_tag_history(namespace_name, repository_name, page, size, | ||||
|                                                    specific_tag) == expected | ||||
| 
 | ||||
| 
 | ||||
| def get_repo_mock(monkeypatch, return_value): | ||||
|   def return_return_value(namespace_name, repository_name): | ||||
|     return return_value | ||||
|  | @ -238,60 +166,6 @@ def test_get_parent_images_empty_parent_images(get_monkeypatch): | |||
|   assert images == [] | ||||
| 
 | ||||
| 
 | ||||
| def compare_images(parent_image, attr_dict): | ||||
|   for field in parent_image._fields: | ||||
|     assert getattr(parent_image, field) == getattr(attr_dict, field) | ||||
| 
 | ||||
| 
 | ||||
| def test_get_parent_images(get_monkeypatch): | ||||
|   get_image_by_id_mock = Mock() | ||||
|   get_monkeypatch.setattr(model.image, 'get_image_by_id', get_image_by_id_mock) | ||||
| 
 | ||||
|   def fake_list(): | ||||
|     return [] | ||||
| 
 | ||||
|   image_one = AttrDict({ | ||||
|     'docker_image_id': 'docker_image_id', | ||||
|     'created': 'created_one', | ||||
|     'comment': 'comment_one', | ||||
|     'command': 'command_one', | ||||
|     'storage': AttrDict({ | ||||
|       'image_size': 'image_size_one', | ||||
|       'uploading': 'uploading_one' | ||||
|     }), | ||||
|     'ancestors': 'one/two/three', | ||||
|     'ancestor_id_list': fake_list | ||||
|   }) | ||||
|   image_two = AttrDict({ | ||||
|     'docker_image_id': 'docker_image_id_two', | ||||
|     'created': 'created', | ||||
|     'comment': 'comment', | ||||
|     'command': 'command', | ||||
|     'storage': AttrDict({ | ||||
|       'image_size': 'image_size', | ||||
|       'uploading': 'uploading' | ||||
|     }), | ||||
|     'ancestors': 'four/five/six', | ||||
|     'ancestor_id_list': fake_list | ||||
|   }) | ||||
| 
 | ||||
|   get_parent_images_mock = Mock(return_value=[image_one, image_two]) | ||||
|   get_monkeypatch.setattr(model.image, 'get_parent_images', get_parent_images_mock) | ||||
|   get_monkeypatch.setattr(model.image, 'get_image_by_id', Mock()) | ||||
| 
 | ||||
|   images = pre_oci_model.get_parent_images('namespace_name', 'repository_name', 'tag_name') | ||||
|   image_one.ancestor_id_list = [] | ||||
|   image_one.storage_image_size = 'image_size_one' | ||||
|   image_one.storage_uploading = 'uploading_one' | ||||
|   image_one.ancestor_length = 13 | ||||
|   image_two.ancestor_id_list = [] | ||||
|   image_two.storage_uploading = 'uploading' | ||||
|   image_two.storage_image_size = 'image_size' | ||||
|   image_two.ancestor_length = 13 | ||||
|   compare_images(images[0], image_one) | ||||
|   compare_images(images[1], image_two) | ||||
| 
 | ||||
| 
 | ||||
| def test_list_repository_tags(get_monkeypatch): | ||||
|   mock = Mock(return_value=[]) | ||||
| 
 | ||||
|  |  | |||
		Reference in a new issue