From fc4b3642d3934c0f4f8ecb2abd22175d6bfd0109 Mon Sep 17 00:00:00 2001 From: Charlton Austin Date: Thu, 6 Jul 2017 14:50:30 -0400 Subject: [PATCH 1/3] refactor(endpoints/api/tag): refactor code for v22 this decouples the database models from the api [TESTING->locally with docker compose] Issue: https://coreosdev.atlassian.net/browse/QUAY-632 - [ ] It works! - [ ] Comments provide sufficient explanations for the next contributor - [ ] Tests cover changes and corner cases - [ ] Follows Quay syntax patterns and format --- data/model/log.py | 7 +- endpoints/api/image.py | 27 +++ endpoints/api/tag.py | 65 +++---- endpoints/api/tag_models_interface.py | 89 ++++++++- endpoints/api/tag_models_pre_oci.py | 101 +++++++++- endpoints/api/test/test_models_pre_oci.py | 225 +++++++++++++++++++++- endpoints/api/test/test_tag.py | 36 ++-- 7 files changed, 483 insertions(+), 67 deletions(-) diff --git a/data/model/log.py b/data/model/log.py index b8dd0328c..a1cd195e2 100644 --- a/data/model/log.py +++ b/data/model/log.py @@ -6,6 +6,7 @@ from peewee import JOIN_LEFT_OUTER, fn, PeeweeException from datetime import datetime, timedelta from cachetools import lru_cache +import data from data.database import LogEntry, LogEntryKind, User, RepositoryActionCount, db from data.model import config, user, DataModelException @@ -110,7 +111,11 @@ def log_action(kind_name, user_or_organization_name, performer=None, repository= performer = performer.id if repository is not None: - repository = repository.id + if hasattr(repository, 'namespace_name') and hasattr(repository, 'repository_name'): + maybe_repo = data.model.repository.get_repository(repository.namespace_name, repository.repository_name) + repository = maybe_repo.id if maybe_repo else None + elif hasattr(repository, 'id'): + repository = repository.id kind = _get_log_entry_kind(kind_name) metadata_json = json.dumps(metadata, default=_json_serialize) diff --git a/endpoints/api/image.py b/endpoints/api/image.py index 35c346ff1..f1b67e6d8 100644 --- a/endpoints/api/image.py +++ b/endpoints/api/image.py @@ -9,6 +9,33 @@ from endpoints.exception import NotFound from data import model +def image_view_pre_oci(image, image_map, include_ancestors=True): + command = image.command + + def docker_id(aid): + if aid not in image_map: + return '' + + return image_map[aid].docker_image_id + + image_data = { + 'id': image.docker_image_id, + 'created': format_date(image.created), + 'comment': image.comment, + 'command': json.loads(command) if command else None, + 'size': image.storage_image_size, + 'uploading': image.storage_uploading, + 'sort_index': image.ancestor_length, + } + + if include_ancestors: + # Calculate the ancestors string, with the DBID's replaced with the docker IDs. + ancestors = [docker_id(a) for a in image.ancestor_id_list] + image_data['ancestors'] = '/{0}/'.format('/'.join(ancestors)) + + return image_data + + def image_view(image, image_map, include_ancestors=True): command = image.command diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index 1422b2971..2298de8aa 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -3,12 +3,13 @@ from flask import request, abort from auth.auth_context import get_authenticated_user -from data import model +from data.model import DataModelException from endpoints.api import (resource, nickname, require_repo_read, require_repo_write, RepositoryParamResource, log_action, validate_json_request, path_param, parse_args, query_param, truthy_bool, disallow_for_app_repositories) -from endpoints.api.image import image_view -from endpoints.api.tag_models_pre_oci import pre_oci_model +from endpoints.api.image import image_view_pre_oci +from endpoints.api.tag_models_interface import Repository +from endpoints.api.tag_models_pre_oci import pre_oci_model as model from endpoints.exception import NotFound from endpoints.v2.manifest import _generate_and_store_manifest from util.names import TAG_ERROR, TAG_REGEX @@ -51,7 +52,7 @@ class ListRepositoryTags(RepositoryParamResource): page = max(1, parsed_args.get('page', 1)) limit = min(100, max(1, parsed_args.get('limit', 50))) - tag_history = pre_oci_model.list_repository_tag_history(namespace_name=namespace, + tag_history = model.list_repository_tag_history(namespace_name=namespace, repository_name=repository, page=page, size=limit, specific_tag=specific_tag) @@ -74,7 +75,7 @@ class RepositoryTag(RepositoryParamResource): 'MoveTag': { 'type': 'object', 'description': 'Description of to which image a new or existing tag should point', - 'required': ['image',], + 'required': ['image', ], 'properties': { 'image': { 'type': 'string', @@ -95,20 +96,12 @@ class RepositoryTag(RepositoryParamResource): abort(400, TAG_ERROR) image_id = request.get_json()['image'] - image = model.image.get_repo_image(namespace, repository, image_id) - if not image: + repo = model.get_repo(namespace, repository, image_id) + if not repo: raise NotFound() - original_image_id = None - try: - original_tag_image = model.tag.get_repo_tag_image(image.repository, tag) - if original_tag_image: - original_image_id = original_tag_image.docker_image_id - except model.DataModelException: - # This is a new tag. - pass - - model.tag.create_or_update_tag(namespace, repository, tag, image_id) + original_image_id = model.get_repo_tag_image(repo, tag) + model.create_or_update_tag(namespace, repository, tag, image_id) username = get_authenticated_user().username log_action('move_tag' if original_image_id else 'create_tag', namespace, { @@ -118,7 +111,7 @@ class RepositoryTag(RepositoryParamResource): 'namespace': namespace, 'image': image_id, 'original_image': original_image_id - }, repo=model.repository.get_repository(namespace, repository)) + }, repo=repo) _generate_and_store_manifest(namespace, repository, tag) @@ -129,14 +122,14 @@ class RepositoryTag(RepositoryParamResource): @nickname('deleteFullTag') def delete(self, namespace, repository, tag): """ Delete the specified repository tag. """ - model.tag.delete_tag(namespace, repository, tag) + model.delete_tag(namespace, repository, tag) username = get_authenticated_user().username log_action('delete_tag', namespace, {'username': username, 'repo': repository, 'namespace': namespace, - 'tag': tag}, repo=model.repository.get_repository(namespace, repository)) + 'tag': tag}, repo=Repository(namespace_name=namespace, repository_name=repository)) return '', 204 @@ -156,17 +149,15 @@ class RepositoryTagImages(RepositoryParamResource): def get(self, namespace, repository, tag, parsed_args): """ List the images for the specified repository tag. """ try: - tag_image = model.tag.get_tag_image(namespace, repository, tag) - except model.DataModelException: + tag_image = model.get_repo_tag_image(Repository(namespace_name=namespace, repository_name=repository), tag) + except DataModelException: raise NotFound() - parent_images = model.image.get_parent_images(namespace, repository, tag_image) - image_map = {} - - image_map[str(tag_image.id)] = tag_image + 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.id)] = image + image_map[str(image.docker_image_id)] = image image_map_all = dict(image_map) all_images = [tag_image] + list(parent_images) @@ -174,13 +165,13 @@ class RepositoryTagImages(RepositoryParamResource): # Filter the images returned to those not found in the ancestry of any of the other tags in # the repository. if parsed_args['owned']: - all_tags = model.tag.list_repository_tags(namespace, repository) + all_tags = model.list_repository_tags(namespace, repository) for current_tag in all_tags: if current_tag.name == tag: continue # Remove the tag's image ID. - tag_image_id = str(current_tag.image_id) + tag_image_id = str(current_tag.docker_image_id) image_map.pop(tag_image_id, None) # Remove any ancestors: @@ -189,8 +180,8 @@ class RepositoryTagImages(RepositoryParamResource): return { 'images': [ - image_view(image, image_map_all) for image in all_images - if not parsed_args['owned'] or (str(image.id) in image_map) + image_view_pre_oci(image, image_map_all) for image in all_images + if not parsed_args['owned'] or (str(image.docker_image_id) in image_map) ] } @@ -204,7 +195,7 @@ class RestoreTag(RepositoryParamResource): 'RestoreTag': { 'type': 'object', 'description': 'Restores a tag to a specific image', - 'required': ['image',], + 'required': ['image', ], 'properties': { 'image': { 'type': 'string', @@ -224,7 +215,6 @@ class RestoreTag(RepositoryParamResource): @validate_json_request('RestoreTag') def post(self, namespace, repository, tag): """ Restores a repository tag back to a previous image in the repository. """ - repo = model.repository.get_repository(namespace, repository) # Restore the tag back to the previous image. image_id = request.get_json()['image'] @@ -238,18 +228,17 @@ class RestoreTag(RepositoryParamResource): 'tag': tag, 'image': image_id, } - + repo = Repository(namespace, repository) if manifest_digest is not None: - existing_image = model.tag.restore_tag_to_manifest(repo, tag, manifest_digest) + existing_image = model.restore_tag_to_manifest(repo, tag, manifest_digest) else: - existing_image = model.tag.restore_tag_to_image(repo, tag, image_id) + existing_image = model.restore_tag_to_image(repo, tag, image_id) _generate_and_store_manifest(namespace, repository, tag) if existing_image is not None: log_data['original_image'] = existing_image.docker_image_id - log_action('revert_tag', namespace, log_data, repo=model.repository.get_repository(namespace, - repository)) + log_action('revert_tag', namespace, log_data, repo=repo) return { 'image_id': image_id, diff --git a/endpoints/api/tag_models_interface.py b/endpoints/api/tag_models_interface.py index c98737f9a..f93ce0b6f 100644 --- a/endpoints/api/tag_models_interface.py +++ b/endpoints/api/tag_models_interface.py @@ -5,10 +5,8 @@ from six import add_metaclass class Tag( - namedtuple('Tag', [ - 'name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', 'manifest_list', - 'docker_image_id' - ])): + namedtuple('Tag', ['name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', + 'manifest_list', 'docker_image_id'])): """ Tag represents a name to an image. :type name: string @@ -29,6 +27,30 @@ class RepositoryTagHistory(namedtuple('RepositoryTagHistory', ['tags', 'more'])) """ +class Repository(namedtuple('Repository', ['namespace_name', 'repository_name'])): + """ + Repository a single quay repository + :type namespace_name: string + :type repository_name: string + """ + + +class Image(namedtuple('Image', ['docker_image_id', 'created', 'comment', 'command', 'storage_image_size', + 'storage_uploading', 'ancestor_length', 'ancestor_id_list'])): + """ + Image + :type docker_image_id: string + :type created: datetime + :type comment: string + :type command: string + :type storage_image_size: int + :type storage_uploading: boolean + :type ancestor_length: int + :type ancestor_id_list: [int] + + """ + + @add_metaclass(ABCMeta) class TagDataInterface(object): """ @@ -36,8 +58,63 @@ class TagDataInterface(object): """ @abstractmethod - def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, - specific_tag=None): + def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, specific_tag=None): """ Returns a RepositoryTagHistory with a list of historic tags and whether there are more tags then returned. """ + + @abstractmethod + def get_repo(self, namespace_name, repository_name, docker_image_id): + """ + Returns a repository associated with the given namespace, repository, and docker_image_id + """ + + @abstractmethod + def get_repo_tag_image(self, repository, tag_name): + """ + Returns an image associated with the repository and tag_name + """ + + @abstractmethod + def create_or_update_tag(self, namespace_name, repository_name, tag_name, docker_image_id): + """ + Returns the repository tag if it is created. + """ + + @abstractmethod + def delete_tag(self, namespace_name, repository_name, tag_name): + """ + Returns the tag for the given namespace and repository if it was created + """ + + @abstractmethod + def get_parent_images(self, namespace, repository, tag_name): + """ + Returns a list of the parent images for the namespace, repository and tag specified. + """ + + @abstractmethod + def list_repository_tags(self, namespace_name, repository_name): + """ + Returns a list of all tags associated with namespace_nam and repository_name + """ + + @abstractmethod + def get_repository(self, namespace_name, repository_name): + """ + Returns the repository associated with the namespace_name and repository_name + """ + + @abstractmethod + def restore_tag_to_manifest(self, repository_name, tag_name, manifest_digest): + """ + Returns the existing repo tag image if it exists or else returns None. + Side effects include adding the tag with associated name to the manifest_digest in the named repo. + """ + + @abstractmethod + def restore_tag_to_image(self, repository_name, tag_name, image_id): + """ + Returns the existing repo tag image if it exists or else returns None + Side effects include adding the tag with associated name to the image with the associated id in the named repo. + """ diff --git a/endpoints/api/tag_models_pre_oci.py b/endpoints/api/tag_models_pre_oci.py index a1842b036..65fc77496 100644 --- a/endpoints/api/tag_models_pre_oci.py +++ b/endpoints/api/tag_models_pre_oci.py @@ -1,5 +1,6 @@ from data import model -from endpoints.api.tag_models_interface import TagDataInterface, Tag, RepositoryTagHistory +from data.model import DataModelException, InvalidImageException +from endpoints.api.tag_models_interface import TagDataInterface, Tag, RepositoryTagHistory, Repository, Image class PreOCIModel(TagDataInterface): @@ -8,11 +9,11 @@ class PreOCIModel(TagDataInterface): before it was changed to support the OCI specification. """ - def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, - specific_tag=None): + def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, specific_tag=None): repository = model.repository.get_repository(namespace_name, repository_name) if repository is None: return None + tags, manifest_map, more = model.tag.list_repository_tag_history(repository, page, size, specific_tag) repository_tag_history = [] @@ -20,11 +21,97 @@ class PreOCIModel(TagDataInterface): manifest_list = None if tag.id in manifest_map: manifest_list = manifest_map[tag.id] - repository_tag_history.append( - Tag(name=tag.name, 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)) + + repository_tag_history.append(convert_tag(tag, manifest_list)) + return RepositoryTagHistory(tags=repository_tag_history, more=more) + def get_repo(self, namespace_name, repository_name, docker_image_id): + image = model.image.get_repo_image(namespace_name, repository_name, docker_image_id) + if image is None: + return None + + return Repository(image.repository.namespace_user, image.repository.name) + + def get_repo_tag_image(self, repository, tag_name): + repo = model.repository.get_repository(repository.namespace_name, repository.repository_name) + if repo is None: + return None + + try: + image = model.tag.get_repo_tag_image(repo, tag_name) + except DataModelException: + return None + + return convert_image(image) + + def create_or_update_tag(self, namespace_name, repository_name, tag_name, docker_image_id): + return model.tag.create_or_update_tag(namespace_name, repository_name, tag_name, docker_image_id) + + def delete_tag(self, namespace_name, repository_name, tag_name): + return model.tag.delete_tag(namespace_name, repository_name, tag_name) + + def get_parent_images(self, namespace_name, repository_name, docker_image_id): + try: + image = model.image.get_image_by_id(namespace_name, repository_name, docker_image_id) + except InvalidImageException: + return [] + + parent_tags = model.image.get_parent_images(namespace_name, repository_name, image) + return_tags = [] + for image in parent_tags: + return_tags.append(convert_image(image)) + + return return_tags + + def list_repository_tags(self, namespace_name, repository_name): + tags = model.tag.list_repository_tags(namespace_name, repository_name) + new_tags = [] + for tag in tags: + new_tags.append(convert_tag(tag)) + return new_tags + + def get_repository(self, namespace_name, repository_name): + repo = model.repository.get_repository(namespace_name, repository_name) + if repo is None: + return None + + return Repository(namespace_name=namespace_name, repository_name=repository_name) + + def restore_tag_to_manifest(self, repository, tag_name, manifest_digest): + repo = model.repository.get_repository(repository.namespace_name, repository.repository_name) + if repo is None: + return None + + image = model.tag.restore_tag_to_manifest(repo, tag_name, manifest_digest) + if image is None: + return None + + return convert_image(image) + + def restore_tag_to_image(self, repository, tag_name, image_id): + repo = model.repository.get_repository(repository.namespace_name, repository.repository_name) + if repo is None: + return None + + image = model.tag.restore_tag_to_image(repo, tag_name, image_id) + if image is None: + return None + + return convert_image(image) + + +def convert_image(database_image): + return Image(docker_image_id=database_image.docker_image_id, created=database_image.created, + comment=database_image.comment, command=database_image.command, + storage_image_size=database_image.storage.image_size, storage_uploading=database_image.storage.uploading, + ancestor_length=len(database_image.ancestors), ancestor_id_list=database_image.ancestor_id_list()) + + +def convert_tag(tag, manifest_list=None): + return Tag(name=tag.name, 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) + pre_oci_model = PreOCIModel() diff --git a/endpoints/api/test/test_models_pre_oci.py b/endpoints/api/test/test_models_pre_oci.py index 7481511a0..ecf343fb6 100644 --- a/endpoints/api/test/test_models_pre_oci.py +++ b/endpoints/api/test/test_models_pre_oci.py @@ -1,9 +1,12 @@ import pytest -from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag -from mock import Mock + +from data.model import DataModelException, InvalidImageException +from endpoints.api.tag_models_interface import RepositoryTagHistory, Tag, Repository +from mock import Mock, call from data import model from endpoints.api.tag_models_pre_oci import pre_oci_model +from util.morecollections import AttrDict EMPTY_REPOSITORY = 'empty_repository' EMPTY_NAMESPACE = 'empty_namespace' @@ -101,3 +104,221 @@ def test_list_repository_tag_history(expected, namespace_name, repository_name, size, specific_tag) assert pre_oci_model.list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag) == expected + + +def get_repo_image_mock(monkeypatch, return_value): + def return_return_value(namespace_name, repository_name, image_id): + return return_value + + monkeypatch.setattr(model.image, 'get_repo_image', return_return_value) + + +def test_get_repo_not_exists(get_monkeypatch): + namespace_name = 'namespace_name' + repository_name = 'repository_name' + image_id = 'image_id' + get_repo_image_mock(get_monkeypatch, None) + + repo = pre_oci_model.get_repo(namespace_name, repository_name, image_id) + + assert repo is None + + +def test_get_repo_exists(get_monkeypatch): + namespace_name = 'namespace_name' + repository_name = 'repository_name' + image_id = 'image_id' + mock = Mock() + mock.namespace_user = namespace_name + mock.name = repository_name + mock.repository = mock + get_repo_image_mock(get_monkeypatch, mock) + + repo = pre_oci_model.get_repo(namespace_name, repository_name, image_id) + + assert repo is not None + assert repo.repository_name == repository_name + assert repo.namespace_name == namespace_name + + +def get_repository_mock(monkeypatch, return_value): + def return_return_value(namespace_name, repository_name, kind_filter=None): + return return_value + + monkeypatch.setattr(model.repository, 'get_repository', return_return_value) + + +def get_repo_tag_image_mock(monkeypatch, return_value): + def return_return_value(repo, tag_name, include_storage=False): + return return_value + + monkeypatch.setattr(model.tag, 'get_repo_tag_image', return_return_value) + + +def test_get_repo_tag_image_with_repo_and_repo_tag(get_monkeypatch): + mock_storage = Mock() + mock_image = Mock() + mock_image.docker_image_id = 'some docker image id' + mock_image.created = 1235 + mock_image.comment = 'some comment' + mock_image.command = 'some command' + mock_image.storage = mock_storage + mock_image.ancestors = [] + + get_repository_mock(get_monkeypatch, mock_image) + get_repo_tag_image_mock(get_monkeypatch, mock_image) + + image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + + assert image is not None + assert image.docker_image_id == 'some docker image id' + + +def test_get_repo_tag_image_without_repo(get_monkeypatch): + get_repository_mock(get_monkeypatch, None) + + image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + + assert image is None + + +def test_get_repo_tag_image_without_repo_tag_image(get_monkeypatch): + mock = Mock() + mock.docker_image_id = 'some docker image id' + get_repository_mock(get_monkeypatch, mock) + + def raise_exception(repo, tag_name, include_storage=False): + raise DataModelException() + + get_monkeypatch.setattr(model.tag, 'get_repo_tag_image', raise_exception) + + image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + + assert image is None + + +def test_create_or_update_tag(get_monkeypatch): + mock = Mock() + get_monkeypatch.setattr(model.tag, 'create_or_update_tag', mock) + + pre_oci_model.create_or_update_tag('namespace_name', 'repository_name', 'tag_name', 'docker_image_id') + + assert mock.call_count == 1 + assert mock.call_args == call('namespace_name', 'repository_name', 'tag_name', 'docker_image_id') + + +def test_delete_tag(get_monkeypatch): + mock = Mock() + get_monkeypatch.setattr(model.tag, 'delete_tag', mock) + + pre_oci_model.delete_tag('namespace_name', 'repository_name', 'tag_name') + + assert mock.call_count == 1 + assert mock.call_args == call('namespace_name', 'repository_name', 'tag_name') + + +def test_get_parent_images_with_exception(get_monkeypatch): + mock = Mock(side_effect=InvalidImageException) + + get_monkeypatch.setattr(model.image, 'get_image_by_id', mock) + + images = pre_oci_model.get_parent_images('namespace_name', 'repository_name', 'tag_name') + assert images == [] + + +def test_get_parent_images_empty_parent_images(get_monkeypatch): + get_image_by_id_mock = Mock() + get_monkeypatch.setattr(model.image, 'get_image_by_id', get_image_by_id_mock) + + get_parent_images_mock = Mock(return_value=[]) + get_monkeypatch.setattr(model.image, 'get_parent_images', get_parent_images_mock) + + images = pre_oci_model.get_parent_images('namespace_name', 'repository_name', 'tag_name') + 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=[]) + + get_monkeypatch.setattr(model.tag, 'list_repository_tags', mock) + + pre_oci_model.list_repository_tags('namespace_name', 'repository_name') + + mock.assert_called_once_with('namespace_name', 'repository_name') + + +def test_get_repository(get_monkeypatch): + mock = Mock() + + get_monkeypatch.setattr(model.repository, 'get_repository', mock) + + pre_oci_model.get_repository('namespace_name', 'repository_name') + + mock.assert_called_once_with('namespace_name', 'repository_name') + + +def test_tag_to_manifest(get_monkeypatch): + repo_mock = Mock() + restore_tag_mock = Mock(return_value=None) + get_repository_mock = Mock(return_value=repo_mock) + + get_monkeypatch.setattr(model.tag, 'restore_tag_to_manifest', restore_tag_mock) + get_monkeypatch.setattr(model.repository, 'get_repository', get_repository_mock) + + pre_oci_model.restore_tag_to_manifest(Repository('namespace', 'repository'), 'tag_name', 'manifest_digest') + + get_repository_mock.assert_called_once_with('namespace', 'repository') + restore_tag_mock.assert_called_once_with(repo_mock, 'tag_name', 'manifest_digest') + + +def test__tag_to_image(get_monkeypatch): + repo_mock = Mock() + restore_tag_mock = Mock(return_value=None) + get_repository_mock = Mock(return_value=repo_mock) + + + get_monkeypatch.setattr(model.tag, 'restore_tag_to_image', restore_tag_mock) + get_monkeypatch.setattr(model.repository, 'get_repository', get_repository_mock) + + pre_oci_model.restore_tag_to_image(Repository('namespace', 'repository'), 'tag_name', 'image_id') + + get_repository_mock.assert_called_once_with('namespace', 'repository') + restore_tag_mock.assert_called_once_with(repo_mock, 'tag_name', 'image_id') diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index c8f578f53..30f6dd76e 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -4,7 +4,7 @@ import pytest from mock import patch, Mock, MagicMock, call - +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 @@ -18,26 +18,36 @@ from test.fixtures import * @pytest.fixture() def get_repo_image(): def mock_callable(namespace, repository, image_id): - img = Mock(repository='fetched_repository') if image_id == 'image1' else None + mock = Mock(namespace_user='devtable') + mock.name = 'simple' + img = Mock(repository=mock, docker_image_id=12) if image_id == 'image1' else None return img - with patch('endpoints.api.tag.model.image.get_repo_image', side_effect=mock_callable) as mk: + with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', side_effect=mock_callable) as mk: yield mk @pytest.fixture() def get_repository(): - with patch('endpoints.api.tag.model.image.get_repo_image', return_value='mock_repo') as mk: + with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', return_value='mock_repo') as mk: yield mk @pytest.fixture() def get_repo_tag_image(): def mock_get_repo_tag_image(repository, tag): - tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None - return tag_img + storage_mock = Mock(image_size=1234, uploading='uploading') - with patch('endpoints.api.tag.model.tag.get_repo_tag_image', + def fake_ancestor_id_list(): + return [] + + if tag == 'existing-tag': + return Mock(docker_image_id='mock_docker_image_id', created=12345, comment='comment', command='command', + storage=storage_mock, ancestors=[], ancestor_id_list=fake_ancestor_id_list) + else: + raise DataModelException('Unable to find image for tag.') + + with patch('endpoints.api.tag_models_pre_oci.model.tag.get_repo_tag_image', side_effect=mock_get_repo_tag_image): yield @@ -48,7 +58,7 @@ def restore_tag_to_manifest(): tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None return tag_img - with patch('endpoints.api.tag.model.tag.restore_tag_to_manifest', + with patch('endpoints.api.tag_models_pre_oci.model.tag.restore_tag_to_manifest', side_effect=mock_restore_tag_to_manifest): yield @@ -59,14 +69,14 @@ def restore_tag_to_image(): tag_img = Mock(docker_image_id='mock_docker_image_id') if tag == 'existing-tag' else None return tag_img - with patch('endpoints.api.tag.model.tag.restore_tag_to_image', + with patch('endpoints.api.tag_models_pre_oci.model.tag.restore_tag_to_image', side_effect=mock_restore_tag_to_image): yield @pytest.fixture() def create_or_update_tag(): - with patch('endpoints.api.tag.model.tag.create_or_update_tag') as mk: + with patch('endpoints.api.tag_models_pre_oci.model.tag.create_or_update_tag') as mk: yield mk @@ -95,7 +105,7 @@ def list_repository_tag_history(): Tag(name='Second Tag', image='second image', reversion=True, lifetime_start_ts=10, lifetime_end_ts=100, manifest_list=[], docker_image_id='second docker image id')], more=False) - with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=list_repository_tag_history): + with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=list_repository_tag_history): yield @@ -104,7 +114,7 @@ def find_no_repo_tag_history(): def list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag): return None - with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=list_repository_tag_history): + with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=list_repository_tag_history): yield @@ -178,7 +188,7 @@ def test_repo_tag_history_param_parse(specific_tag, page, limit, expected_specif mock = MagicMock() mock.return_value = RepositoryTagHistory(tags=[], more=False) - with patch('endpoints.api.tag.pre_oci_model.list_repository_tag_history', side_effect=mock): + with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=mock): params = {'repository': 'devtable/simple', 'specificTag': specific_tag, 'page': page, 'limit': limit} conduct_api_call(authd_client, ListRepositoryTags, 'get', params) From 897a091692736e0be5f239be96692363376150e8 Mon Sep 17 00:00:00 2001 From: Charlton Austin Date: Mon, 10 Jul 2017 09:46:02 -0400 Subject: [PATCH 2/3] style(data+endpoints): ran yapf ### Description of Changes ran yapf for the branch [TESTING->locally using docker compose] Issue: https://coreosdev.atlassian.net/browse/QUAY-632 ## Reviewer Checklist - [ ] It works! - [ ] Comments provide sufficient explanations for the next contributor - [ ] Tests cover changes and corner cases - [ ] Follows Quay syntax patterns and format --- data/model/log.py | 40 ++++------- endpoints/api/image.py | 7 +- endpoints/api/tag.py | 11 +-- endpoints/api/tag_models_interface.py | 16 +++-- endpoints/api/tag_models_pre_oci.py | 12 ++-- endpoints/api/test/test_models_pre_oci.py | 48 +++++++++---- endpoints/api/test/test_tag.py | 88 +++++++++++++++-------- 7 files changed, 135 insertions(+), 87 deletions(-) diff --git a/data/model/log.py b/data/model/log.py index a1cd195e2..6d8edd6ba 100644 --- a/data/model/log.py +++ b/data/model/log.py @@ -14,11 +14,10 @@ logger = logging.getLogger(__name__) ACTIONS_ALLOWED_WITHOUT_AUDIT_LOGGING = ['pull_repo'] + def _logs_query(selections, start_time, end_time, performer=None, repository=None, namespace=None, ignore=None): - joined = (LogEntry - .select(*selections) - .switch(LogEntry) + joined = (LogEntry.select(*selections).switch(LogEntry) .where(LogEntry.datetime >= start_time, LogEntry.datetime < end_time)) if repository: @@ -75,14 +74,12 @@ def get_logs_query(start_time, end_time, performer=None, repository=None, namesp selections.append(Account) query = _logs_query(selections, start_time, end_time, performer, repository, namespace, ignore) - query = (query.switch(LogEntry) - .join(Performer, JOIN_LEFT_OUTER, - on=(LogEntry.performer == Performer.id).alias('performer'))) + query = (query.switch(LogEntry).join(Performer, JOIN_LEFT_OUTER, + on=(LogEntry.performer == Performer.id).alias('performer'))) if namespace is None and repository is None: - query = (query.switch(LogEntry) - .join(Account, JOIN_LEFT_OUTER, - on=(LogEntry.account == Account.id).alias('account'))) + query = (query.switch(LogEntry).join(Account, JOIN_LEFT_OUTER, + on=(LogEntry.account == Account.id).alias('account'))) return query @@ -94,8 +91,8 @@ def _json_serialize(obj): return obj -def log_action(kind_name, user_or_organization_name, performer=None, repository=None, - ip=None, metadata={}, timestamp=None): +def log_action(kind_name, user_or_organization_name, performer=None, repository=None, ip=None, + metadata={}, timestamp=None): if not timestamp: timestamp = datetime.today() @@ -112,7 +109,8 @@ def log_action(kind_name, user_or_organization_name, performer=None, repository= if repository is not None: if hasattr(repository, 'namespace_name') and hasattr(repository, 'repository_name'): - maybe_repo = data.model.repository.get_repository(repository.namespace_name, repository.repository_name) + maybe_repo = data.model.repository.get_repository(repository.namespace_name, + repository.repository_name) repository = maybe_repo.id if maybe_repo else None elif hasattr(repository, 'id'): repository = repository.id @@ -139,15 +137,10 @@ def log_action(kind_name, user_or_organization_name, performer=None, repository= raise - def get_stale_logs_start_id(): """ Gets the oldest log entry. """ try: - return (LogEntry - .select(LogEntry.id) - .order_by(LogEntry.id) - .limit(1) - .tuples())[0][0] + return (LogEntry.select(LogEntry.id).order_by(LogEntry.id).limit(1).tuples())[0][0] except IndexError: return None @@ -155,9 +148,7 @@ def get_stale_logs_start_id(): def get_stale_logs_cutoff_id(cutoff_date): """ Gets the most recent ID created before the cutoff_date. """ try: - return (LogEntry - .select(fn.Max(LogEntry.id)) - .where(LogEntry.datetime <= cutoff_date) + return (LogEntry.select(fn.Max(LogEntry.id)).where(LogEntry.datetime <= cutoff_date) .tuples())[0][0] except IndexError: return None @@ -184,12 +175,11 @@ def get_repositories_action_sums(repository_ids): # Filter the join to recent entries only. last_week = datetime.now() - timedelta(weeks=1) - tuples = (RepositoryActionCount - .select(RepositoryActionCount.repository, fn.Sum(RepositoryActionCount.count)) + tuples = (RepositoryActionCount.select(RepositoryActionCount.repository, + fn.Sum(RepositoryActionCount.count)) .where(RepositoryActionCount.repository << repository_ids) .where(RepositoryActionCount.date >= last_week) - .group_by(RepositoryActionCount.repository) - .tuples()) + .group_by(RepositoryActionCount.repository).tuples()) action_count_map = {} for record in tuples: diff --git a/endpoints/api/image.py b/endpoints/api/image.py index f1b67e6d8..117ddbd81 100644 --- a/endpoints/api/image.py +++ b/endpoints/api/image.py @@ -74,6 +74,7 @@ def historical_image_view(image, image_map): @path_param('repository', 'The full path of the repository. e.g. namespace/name') class RepositoryImageList(RepositoryParamResource): """ Resource for listing repository images. """ + @require_repo_read @nickname('listRepositoryImages') @disallow_for_app_repositories @@ -105,9 +106,7 @@ class RepositoryImageList(RepositoryParamResource): image_json['tags'] = tags_by_docker_id[image_json['id']] return image_json - return { - 'images': [add_tags(image_view(image, image_map)) for image in filtered_images] - } + return {'images': [add_tags(image_view(image, image_map)) for image in filtered_images]} @resource('/v1/repository//image/') @@ -115,6 +114,7 @@ class RepositoryImageList(RepositoryParamResource): @path_param('image_id', 'The Docker image ID') class RepositoryImage(RepositoryParamResource): """ Resource for handling repository images. """ + @require_repo_read @nickname('getImage') @disallow_for_app_repositories @@ -130,4 +130,3 @@ class RepositoryImage(RepositoryParamResource): image_map[current_image.id] = current_image return historical_image_view(image, image_map) - diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index 2298de8aa..b261db74a 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -53,8 +53,8 @@ class ListRepositoryTags(RepositoryParamResource): limit = min(100, max(1, parsed_args.get('limit', 50))) tag_history = model.list_repository_tag_history(namespace_name=namespace, - repository_name=repository, page=page, - size=limit, specific_tag=specific_tag) + repository_name=repository, page=page, + size=limit, specific_tag=specific_tag) if not tag_history: raise NotFound() @@ -75,7 +75,7 @@ class RepositoryTag(RepositoryParamResource): 'MoveTag': { 'type': 'object', 'description': 'Description of to which image a new or existing tag should point', - 'required': ['image', ], + 'required': ['image',], 'properties': { 'image': { 'type': 'string', @@ -149,7 +149,8 @@ class RepositoryTagImages(RepositoryParamResource): def get(self, namespace, repository, tag, parsed_args): """ List the images for the specified repository tag. """ try: - tag_image = model.get_repo_tag_image(Repository(namespace_name=namespace, repository_name=repository), tag) + tag_image = model.get_repo_tag_image( + Repository(namespace_name=namespace, repository_name=repository), tag) except DataModelException: raise NotFound() @@ -195,7 +196,7 @@ class RestoreTag(RepositoryParamResource): 'RestoreTag': { 'type': 'object', 'description': 'Restores a tag to a specific image', - 'required': ['image', ], + 'required': ['image',], 'properties': { 'image': { 'type': 'string', diff --git a/endpoints/api/tag_models_interface.py b/endpoints/api/tag_models_interface.py index f93ce0b6f..aa21dfc61 100644 --- a/endpoints/api/tag_models_interface.py +++ b/endpoints/api/tag_models_interface.py @@ -5,8 +5,10 @@ from six import add_metaclass class Tag( - namedtuple('Tag', ['name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', - 'manifest_list', 'docker_image_id'])): + namedtuple('Tag', [ + 'name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', 'manifest_list', + 'docker_image_id' + ])): """ Tag represents a name to an image. :type name: string @@ -35,8 +37,11 @@ class Repository(namedtuple('Repository', ['namespace_name', 'repository_name']) """ -class Image(namedtuple('Image', ['docker_image_id', 'created', 'comment', 'command', 'storage_image_size', - 'storage_uploading', 'ancestor_length', 'ancestor_id_list'])): +class Image( + namedtuple('Image', [ + 'docker_image_id', 'created', 'comment', 'command', 'storage_image_size', + 'storage_uploading', 'ancestor_length', 'ancestor_id_list' + ])): """ Image :type docker_image_id: string @@ -58,7 +63,8 @@ class TagDataInterface(object): """ @abstractmethod - def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, specific_tag=None): + def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, + specific_tag=None): """ Returns a RepositoryTagHistory with a list of historic tags and whether there are more tags then returned. """ diff --git a/endpoints/api/tag_models_pre_oci.py b/endpoints/api/tag_models_pre_oci.py index 65fc77496..00276a0ca 100644 --- a/endpoints/api/tag_models_pre_oci.py +++ b/endpoints/api/tag_models_pre_oci.py @@ -9,7 +9,8 @@ class PreOCIModel(TagDataInterface): before it was changed to support the OCI specification. """ - def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, specific_tag=None): + def list_repository_tag_history(self, namespace_name, repository_name, page=1, size=100, + specific_tag=None): repository = model.repository.get_repository(namespace_name, repository_name) if repository is None: return None @@ -46,7 +47,8 @@ class PreOCIModel(TagDataInterface): return convert_image(image) def create_or_update_tag(self, namespace_name, repository_name, tag_name, docker_image_id): - return model.tag.create_or_update_tag(namespace_name, repository_name, tag_name, docker_image_id) + return model.tag.create_or_update_tag(namespace_name, repository_name, tag_name, + docker_image_id) def delete_tag(self, namespace_name, repository_name, tag_name): return model.tag.delete_tag(namespace_name, repository_name, tag_name) @@ -104,8 +106,10 @@ class PreOCIModel(TagDataInterface): def convert_image(database_image): return Image(docker_image_id=database_image.docker_image_id, created=database_image.created, comment=database_image.comment, command=database_image.command, - storage_image_size=database_image.storage.image_size, storage_uploading=database_image.storage.uploading, - ancestor_length=len(database_image.ancestors), ancestor_id_list=database_image.ancestor_id_list()) + storage_image_size=database_image.storage.image_size, + storage_uploading=database_image.storage.uploading, + ancestor_length=len(database_image.ancestors), + ancestor_id_list=database_image.ancestor_id_list()) def convert_tag(tag, manifest_list=None): diff --git a/endpoints/api/test/test_models_pre_oci.py b/endpoints/api/test/test_models_pre_oci.py index ecf343fb6..b7c750854 100644 --- a/endpoints/api/test/test_models_pre_oci.py +++ b/endpoints/api/test/test_models_pre_oci.py @@ -168,7 +168,8 @@ def test_get_repo_tag_image_with_repo_and_repo_tag(get_monkeypatch): get_repository_mock(get_monkeypatch, mock_image) get_repo_tag_image_mock(get_monkeypatch, mock_image) - image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + image = pre_oci_model.get_repo_tag_image( + Repository('namespace_name', 'repository_name'), 'tag_name') assert image is not None assert image.docker_image_id == 'some docker image id' @@ -177,7 +178,8 @@ def test_get_repo_tag_image_with_repo_and_repo_tag(get_monkeypatch): def test_get_repo_tag_image_without_repo(get_monkeypatch): get_repository_mock(get_monkeypatch, None) - image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + image = pre_oci_model.get_repo_tag_image( + Repository('namespace_name', 'repository_name'), 'tag_name') assert image is None @@ -192,7 +194,8 @@ def test_get_repo_tag_image_without_repo_tag_image(get_monkeypatch): get_monkeypatch.setattr(model.tag, 'get_repo_tag_image', raise_exception) - image = pre_oci_model.get_repo_tag_image(Repository('namespace_name', 'repository_name'), 'tag_name') + image = pre_oci_model.get_repo_tag_image( + Repository('namespace_name', 'repository_name'), 'tag_name') assert image is None @@ -201,7 +204,8 @@ def test_create_or_update_tag(get_monkeypatch): mock = Mock() get_monkeypatch.setattr(model.tag, 'create_or_update_tag', mock) - pre_oci_model.create_or_update_tag('namespace_name', 'repository_name', 'tag_name', 'docker_image_id') + pre_oci_model.create_or_update_tag('namespace_name', 'repository_name', 'tag_name', + 'docker_image_id') assert mock.call_count == 1 assert mock.call_args == call('namespace_name', 'repository_name', 'tag_name', 'docker_image_id') @@ -249,14 +253,30 @@ def test_get_parent_images(get_monkeypatch): 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}) + 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) @@ -303,7 +323,8 @@ def test_tag_to_manifest(get_monkeypatch): get_monkeypatch.setattr(model.tag, 'restore_tag_to_manifest', restore_tag_mock) get_monkeypatch.setattr(model.repository, 'get_repository', get_repository_mock) - pre_oci_model.restore_tag_to_manifest(Repository('namespace', 'repository'), 'tag_name', 'manifest_digest') + pre_oci_model.restore_tag_to_manifest( + Repository('namespace', 'repository'), 'tag_name', 'manifest_digest') get_repository_mock.assert_called_once_with('namespace', 'repository') restore_tag_mock.assert_called_once_with(repo_mock, 'tag_name', 'manifest_digest') @@ -314,7 +335,6 @@ def test__tag_to_image(get_monkeypatch): restore_tag_mock = Mock(return_value=None) get_repository_mock = Mock(return_value=repo_mock) - get_monkeypatch.setattr(model.tag, 'restore_tag_to_image', restore_tag_mock) get_monkeypatch.setattr(model.repository, 'get_repository', get_repository_mock) diff --git a/endpoints/api/test/test_tag.py b/endpoints/api/test/test_tag.py index 30f6dd76e..1d13d0eae 100644 --- a/endpoints/api/test/test_tag.py +++ b/endpoints/api/test/test_tag.py @@ -23,13 +23,15 @@ def get_repo_image(): img = Mock(repository=mock, docker_image_id=12) if image_id == 'image1' else None return img - with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', side_effect=mock_callable) as mk: + with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', + side_effect=mock_callable) as mk: yield mk @pytest.fixture() def get_repository(): - with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', return_value='mock_repo') as mk: + with patch('endpoints.api.tag_models_pre_oci.model.image.get_repo_image', + return_value='mock_repo') as mk: yield mk @@ -42,8 +44,9 @@ def get_repo_tag_image(): return [] if tag == 'existing-tag': - return Mock(docker_image_id='mock_docker_image_id', created=12345, comment='comment', command='command', - storage=storage_mock, ancestors=[], ancestor_id_list=fake_ancestor_id_list) + return Mock(docker_image_id='mock_docker_image_id', created=12345, comment='comment', + command='command', storage=storage_mock, ancestors=[], + ancestor_id_list=fake_ancestor_id_list) else: raise DataModelException('Unable to find image for tag.') @@ -100,12 +103,14 @@ def authd_client(client): def list_repository_tag_history(): def list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag): return RepositoryTagHistory(tags=[ - Tag(name='First Tag', image='image', reversion=False, lifetime_start_ts=0, lifetime_end_ts=0, manifest_list=[], - docker_image_id='first docker image id'), - Tag(name='Second Tag', image='second image', reversion=True, lifetime_start_ts=10, lifetime_end_ts=100, - manifest_list=[], docker_image_id='second docker image id')], more=False) + Tag(name='First Tag', image='image', reversion=False, lifetime_start_ts=0, lifetime_end_ts=0, + manifest_list=[], docker_image_id='first docker image id'), + Tag(name='Second Tag', image='second image', reversion=True, lifetime_start_ts=10, + lifetime_end_ts=100, manifest_list=[], docker_image_id='second docker image id') + ], more=False) - with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=list_repository_tag_history): + with patch('endpoints.api.tag.model.list_repository_tag_history', + side_effect=list_repository_tag_history): yield @@ -114,7 +119,8 @@ def find_no_repo_tag_history(): def list_repository_tag_history(namespace_name, repository_name, page, size, specific_tag): return None - with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=list_repository_tag_history): + with patch('endpoints.api.tag.model.list_repository_tag_history', + side_effect=list_repository_tag_history): yield @@ -140,23 +146,39 @@ def test_move_tag(test_image, test_tag, expected_status, get_repo_image, get_rep conduct_api_call(authd_client, RepositoryTag, 'put', params, request_body, expected_status) -@pytest.mark.parametrize('namespace, repository, specific_tag, page, limit, expected_response_code, expected', [ - ('devtable', 'simple', None, 1, 10, 200, {'has_additional': False}), - ('devtable', 'simple', None, 1, 10, 200, {'page': 1}), - ('devtable', 'simple', None, 1, 10, 200, {'tags': [{'docker_image_id': 'first docker image id', - 'name': 'First Tag', - 'reversion': False}, - {'docker_image_id': 'second docker image id', - 'end_ts': 100, - 'name': 'Second Tag', - 'reversion': True, - 'start_ts': 10}]}), -]) +@pytest.mark.parametrize( + 'namespace, repository, specific_tag, page, limit, expected_response_code, expected', [ + ('devtable', 'simple', None, 1, 10, 200, { + 'has_additional': False + }), + ('devtable', 'simple', None, 1, 10, 200, { + 'page': 1 + }), + ('devtable', 'simple', None, 1, 10, 200, { + 'tags': [{ + 'docker_image_id': 'first docker image id', + 'name': 'First Tag', + 'reversion': False + }, { + 'docker_image_id': 'second docker image id', + 'end_ts': 100, + 'name': 'Second Tag', + 'reversion': True, + 'start_ts': 10 + }] + }), + ]) def test_list_repository_tags_view_is_correct(namespace, repository, specific_tag, page, limit, - list_repository_tag_history, expected_response_code, expected, - authd_client): - params = {'repository': namespace + '/' + repository, 'specificTag': specific_tag, 'page': page, 'limit': limit} - response = conduct_api_call(authd_client, ListRepositoryTags, 'get', params, expected_code=expected_response_code) + list_repository_tag_history, expected_response_code, + expected, authd_client): + params = { + 'repository': namespace + '/' + repository, + 'specificTag': specific_tag, + 'page': page, + 'limit': limit + } + response = conduct_api_call(authd_client, ListRepositoryTags, 'get', params, + expected_code=expected_response_code) compare_list_history_tags_response(expected, response.json) @@ -183,17 +205,23 @@ def test_no_repo_tag_history(find_no_repo_tag_history, authd_client): ('specific_tag', -1, 101, 'specific_tag', 1, 100), ('specific_tag', 0, 0, 'specific_tag', 1, 1), ]) -def test_repo_tag_history_param_parse(specific_tag, page, limit, expected_specific_tag, expected_page, expected_limit, - authd_client): +def test_repo_tag_history_param_parse(specific_tag, page, limit, expected_specific_tag, + expected_page, expected_limit, authd_client): mock = MagicMock() mock.return_value = RepositoryTagHistory(tags=[], more=False) with patch('endpoints.api.tag.model.list_repository_tag_history', side_effect=mock): - params = {'repository': 'devtable/simple', 'specificTag': specific_tag, 'page': page, 'limit': limit} + params = { + 'repository': 'devtable/simple', + 'specificTag': specific_tag, + 'page': page, + 'limit': limit + } conduct_api_call(authd_client, ListRepositoryTags, 'get', params) assert mock.call_args == call(namespace_name='devtable', repository_name='simple', - page=expected_page, size=expected_limit, specific_tag=expected_specific_tag) + page=expected_page, size=expected_limit, + specific_tag=expected_specific_tag) @pytest.mark.parametrize('test_manifest,test_tag,manifest_generated,expected_status', [ From 131acde31788d085a34645b2662cf8f8cffefdf6 Mon Sep 17 00:00:00 2001 From: Charlton Austin Date: Mon, 10 Jul 2017 14:45:23 -0400 Subject: [PATCH 3/3] refactor(data+endpoints): code review changes this puts the view logic on the object and adds a parameter for logging [TESTING->locally with docker compose] Issue: https://coreosdev.atlassian.net/browse/QUAY-632 - [ ] It works! - [ ] Comments provide sufficient explanations for the next contributor - [ ] Tests cover changes and corner cases - [ ] Follows Quay syntax patterns and format --- data/model/log.py | 7 +-- endpoints/api/__init__.py | 10 ++++- endpoints/api/image.py | 27 ----------- endpoints/api/tag.py | 33 ++++---------- endpoints/api/tag_models_interface.py | 64 +++++++++++++++++++++++---- endpoints/api/tag_models_pre_oci.py | 2 +- 6 files changed, 73 insertions(+), 70 deletions(-) diff --git a/data/model/log.py b/data/model/log.py index 6d8edd6ba..e61f456f7 100644 --- a/data/model/log.py +++ b/data/model/log.py @@ -108,12 +108,7 @@ def log_action(kind_name, user_or_organization_name, performer=None, repository= performer = performer.id if repository is not None: - if hasattr(repository, 'namespace_name') and hasattr(repository, 'repository_name'): - maybe_repo = data.model.repository.get_repository(repository.namespace_name, - repository.repository_name) - repository = maybe_repo.id if maybe_repo else None - elif hasattr(repository, 'id'): - repository = repository.id + repository = repository.id kind = _get_log_entry_kind(kind_name) metadata_json = json.dumps(metadata, default=_json_serialize) diff --git a/endpoints/api/__init__.py b/endpoints/api/__init__.py index 93df3a0eb..655c9f6e3 100644 --- a/endpoints/api/__init__.py +++ b/endpoints/api/__init__.py @@ -353,7 +353,7 @@ def request_error(exception=None, **kwargs): raise InvalidRequest(message, data) -def log_action(kind, user_or_orgname, metadata=None, repo=None): +def log_action(kind, user_or_orgname, metadata=None, repo=None, repo_name=None): if not metadata: metadata = {} @@ -364,8 +364,14 @@ def log_action(kind, user_or_orgname, metadata=None, repo=None): metadata['oauth_token_application'] = oauth_token.application.name performer = get_authenticated_user() + + if repo_name: + repository = model.repository.get_repository(user_or_orgname, repo_name) + else: + repository = repo + model.log.log_action(kind, user_or_orgname, performer=performer, ip=request.remote_addr, - metadata=metadata, repository=repo) + metadata=metadata, repository=repository) def define_json_response(schema_name): diff --git a/endpoints/api/image.py b/endpoints/api/image.py index 117ddbd81..9d0dd6d2e 100644 --- a/endpoints/api/image.py +++ b/endpoints/api/image.py @@ -9,33 +9,6 @@ from endpoints.exception import NotFound from data import model -def image_view_pre_oci(image, image_map, include_ancestors=True): - command = image.command - - def docker_id(aid): - if aid not in image_map: - return '' - - return image_map[aid].docker_image_id - - image_data = { - 'id': image.docker_image_id, - 'created': format_date(image.created), - 'comment': image.comment, - 'command': json.loads(command) if command else None, - 'size': image.storage_image_size, - 'uploading': image.storage_uploading, - 'sort_index': image.ancestor_length, - } - - if include_ancestors: - # Calculate the ancestors string, with the DBID's replaced with the docker IDs. - ancestors = [docker_id(a) for a in image.ancestor_id_list] - image_data['ancestors'] = '/{0}/'.format('/'.join(ancestors)) - - return image_data - - def image_view(image, image_map, include_ancestors=True): command = image.command diff --git a/endpoints/api/tag.py b/endpoints/api/tag.py index b261db74a..8e65257a5 100644 --- a/endpoints/api/tag.py +++ b/endpoints/api/tag.py @@ -7,7 +7,6 @@ from data.model import DataModelException from endpoints.api import (resource, nickname, require_repo_read, require_repo_write, RepositoryParamResource, log_action, validate_json_request, path_param, parse_args, query_param, truthy_bool, disallow_for_app_repositories) -from endpoints.api.image import image_view_pre_oci from endpoints.api.tag_models_interface import Repository from endpoints.api.tag_models_pre_oci import pre_oci_model as model from endpoints.exception import NotFound @@ -15,25 +14,6 @@ from endpoints.v2.manifest import _generate_and_store_manifest from util.names import TAG_ERROR, TAG_REGEX -def tag_view(tag): - tag_info = { - 'name': tag.name, - 'docker_image_id': tag.docker_image_id, - 'reversion': tag.reversion, - } - - if tag.lifetime_start_ts > 0: - tag_info['start_ts'] = tag.lifetime_start_ts - - if tag.lifetime_end_ts > 0: - tag_info['end_ts'] = tag.lifetime_end_ts - - if tag.manifest_list: - tag_info['manifest_digest'] = tag.manifest_list - - return tag_info - - @resource('/v1/repository//tag/') @path_param('repository', 'The full path of the repository. e.g. namespace/name') class ListRepositoryTags(RepositoryParamResource): @@ -60,7 +40,7 @@ class ListRepositoryTags(RepositoryParamResource): raise NotFound() return { - 'tags': [tag_view(tag) for tag in tag_history.tags], + 'tags': [tag.to_dict() for tag in tag_history.tags], 'page': page, 'has_additional': tag_history.more, } @@ -111,7 +91,7 @@ class RepositoryTag(RepositoryParamResource): 'namespace': namespace, 'image': image_id, 'original_image': original_image_id - }, repo=repo) + }, repo_name=repository) _generate_and_store_manifest(namespace, repository, tag) @@ -129,7 +109,7 @@ class RepositoryTag(RepositoryParamResource): {'username': username, 'repo': repository, 'namespace': namespace, - 'tag': tag}, repo=Repository(namespace_name=namespace, repository_name=repository)) + 'tag': tag}, repo_name=repository) return '', 204 @@ -154,6 +134,9 @@ class RepositoryTagImages(RepositoryParamResource): except DataModelException: raise NotFound() + if tag_image is None: + raise NotFound() + parent_images = model.get_parent_images(namespace, repository, tag_image.docker_image_id) image_map = {str(tag_image.docker_image_id): tag_image} @@ -181,7 +164,7 @@ class RepositoryTagImages(RepositoryParamResource): return { 'images': [ - image_view_pre_oci(image, image_map_all) for image in all_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) ] } @@ -239,7 +222,7 @@ class RestoreTag(RepositoryParamResource): if existing_image is not None: log_data['original_image'] = existing_image.docker_image_id - log_action('revert_tag', namespace, log_data, repo=repo) + log_action('revert_tag', namespace, log_data, repo_name=repository) return { 'image_id': image_id, diff --git a/endpoints/api/tag_models_interface.py b/endpoints/api/tag_models_interface.py index aa21dfc61..c54a4cd86 100644 --- a/endpoints/api/tag_models_interface.py +++ b/endpoints/api/tag_models_interface.py @@ -1,14 +1,17 @@ +import json from abc import ABCMeta, abstractmethod from collections import namedtuple from six import add_metaclass +from endpoints.api import format_date + class Tag( - namedtuple('Tag', [ - 'name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', 'manifest_list', - 'docker_image_id' - ])): + namedtuple('Tag', [ + 'name', 'image', 'reversion', 'lifetime_start_ts', 'lifetime_end_ts', 'manifest_list', + 'docker_image_id' + ])): """ Tag represents a name to an image. :type name: string @@ -20,6 +23,24 @@ class Tag( :type docker_image_id: string """ + def to_dict(self): + tag_info = { + 'name': self.name, + 'docker_image_id': self.docker_image_id, + 'reversion': self.reversion, + } + + if self.lifetime_start_ts > 0: + tag_info['start_ts'] = self.lifetime_start_ts + + if self.lifetime_end_ts > 0: + tag_info['end_ts'] = self.lifetime_end_ts + + if self.manifest_list: + tag_info['manifest_digest'] = self.manifest_list + + return tag_info + class RepositoryTagHistory(namedtuple('RepositoryTagHistory', ['tags', 'more'])): """ @@ -38,10 +59,10 @@ class Repository(namedtuple('Repository', ['namespace_name', 'repository_name']) class Image( - namedtuple('Image', [ - 'docker_image_id', 'created', 'comment', 'command', 'storage_image_size', - 'storage_uploading', 'ancestor_length', 'ancestor_id_list' - ])): + namedtuple('Image', [ + 'docker_image_id', 'created', 'comment', 'command', 'storage_image_size', + 'storage_uploading', 'ancestor_length', 'ancestor_id_list' + ])): """ Image :type docker_image_id: string @@ -52,9 +73,34 @@ class Image( :type storage_uploading: boolean :type ancestor_length: int :type ancestor_id_list: [int] - """ + def to_dict(self, image_map, include_ancestors=True): + command = self.command + + def docker_id(aid): + if aid not in image_map: + return '' + + return image_map[aid].docker_image_id + + image_data = { + 'id': self.docker_image_id, + 'created': format_date(self.created), + 'comment': self.comment, + 'command': json.loads(command) if command else None, + 'size': self.storage_image_size, + 'uploading': self.storage_uploading, + 'sort_index': self.ancestor_length, + } + + if include_ancestors: + # Calculate the ancestors string, with the DBID's replaced with the docker IDs. + ancestors = [docker_id(a) for a in self.ancestor_id_list] + image_data['ancestors'] = '/{0}/'.format('/'.join(ancestors)) + + return image_data + @add_metaclass(ABCMeta) class TagDataInterface(object): diff --git a/endpoints/api/tag_models_pre_oci.py b/endpoints/api/tag_models_pre_oci.py index 00276a0ca..1002c78c8 100644 --- a/endpoints/api/tag_models_pre_oci.py +++ b/endpoints/api/tag_models_pre_oci.py @@ -35,7 +35,7 @@ class PreOCIModel(TagDataInterface): return Repository(image.repository.namespace_user, image.repository.name) def get_repo_tag_image(self, repository, tag_name): - repo = model.repository.get_repository(repository.namespace_name, repository.repository_name) + repo = model.repository.get_repository(str(repository.namespace_name), str(repository.repository_name)) if repo is None: return None