From e369b234cae8fbb3f375e1ef2e4882109d57c52f Mon Sep 17 00:00:00 2001 From: Ryan Smith Date: Wed, 4 Aug 2021 08:28:04 -0400 Subject: [PATCH 1/5] Add the .idea/ directory to gitignore for PyCharm users --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 894a44cc..8f1cd5bd 100644 --- a/.gitignore +++ b/.gitignore @@ -102,3 +102,6 @@ venv.bak/ # mypy .mypy_cache/ + +# JetBrains +.idea/ \ No newline at end of file From dbc942e283bbc50bde87b2c3fbe0691286ed2ce6 Mon Sep 17 00:00:00 2001 From: Ryan Smith Date: Wed, 4 Aug 2021 08:34:37 -0400 Subject: [PATCH 2/5] Update failing test to reflect correct expected value based on pulp docs --- tests/repository/test_remove_content.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/repository/test_remove_content.py b/tests/repository/test_remove_content.py index 326c421b..78c14e3f 100644 --- a/tests/repository/test_remove_content.py +++ b/tests/repository/test_remove_content.py @@ -77,7 +77,7 @@ def test_remove_with_type_ids(fast_poller, requests_mocker, client): req[0].url == "https://pulp.example.com/pulp/api/v2/repositories/some-repo/actions/unassociate/" ) - assert req[0].json() == {"type_ids": ["type1", "type2"]} + assert req[0].json() == {"criteria": {"type_ids": ["type1", "type2"]}} def test_remove_loads_units(fast_poller, requests_mocker, client): From d0292fc1503e9d89fc24117133f30b5887bf8343 Mon Sep 17 00:00:00 2001 From: Ryan Smith Date: Wed, 4 Aug 2021 12:43:31 -0400 Subject: [PATCH 3/5] Leverage Criteria, search_for_criteria, and PulpSearch If `type_ids` keyword arg is passed in to `Repository#remove_content` then wrap the list of content type IDs in a `Criteria` object using `_content_type_id` as the field name. This way, when `search_for_criteria` is called, a `PulpSearch` object with its `type_ids` field properly filled out is returned. This can be used to fill out the JSON body of the `unassociate` POST request. --- pubtools/pulplib/_impl/client/client.py | 29 +++++++++++------ pubtools/pulplib/_impl/fake/client.py | 12 +++++-- .../pulplib/_impl/model/repository/base.py | 32 +++++++++++++++++-- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/pubtools/pulplib/_impl/client/client.py b/pubtools/pulplib/_impl/client/client.py index 72e63070..2895c731 100644 --- a/pubtools/pulplib/_impl/client/client.py +++ b/pubtools/pulplib/_impl/client/client.py @@ -1,15 +1,15 @@ -import os -import logging -import threading import hashlib import json +import logging +import os +import threading from functools import partial -import six -from six.moves import StringIO import requests +import six from more_executors import Executors from more_executors.futures import f_map, f_flat_map, f_return, f_proxy, f_sequence +from six.moves import StringIO from ..page import Page from ..criteria import Criteria @@ -17,6 +17,10 @@ from .search import search_for_criteria from .errors import PulpException from .poller import TaskPoller +from .search import search_for_criteria +from ..criteria import Criteria +from ..model import Repository, MaintenanceReport, Distributor, Unit +from ..page import Page from . import retry @@ -470,14 +474,21 @@ def _publish_distributor(self, repo_id, distributor_id, override_config): self._do_request, method="POST", url=url, json=body ) - def _do_unassociate(self, repo_id, type_ids): + def _do_unassociate(self, repo_id, criteria=None): url = os.path.join( self._url, "pulp/api/v2/repositories/%s/actions/unassociate/" % repo_id ) - body = {} - if type_ids is not None: - body["type_ids"] = type_ids + # use type hint=Unit so that if type_ids are the goal here + # then we will get back a properly prepared PulpSearch with + # a populated type_ids field + pulp_search = search_for_criteria(criteria, type_hint=Unit, type_ids_accum=None) + + body = { + "criteria": { + "type_ids": pulp_search.type_ids, + } + } LOG.debug("Submitting %s unassociate: %s", url, body) diff --git a/pubtools/pulplib/_impl/fake/client.py b/pubtools/pulplib/_impl/fake/client.py index 739a4fd2..4c53fa0a 100644 --- a/pubtools/pulplib/_impl/fake/client.py +++ b/pubtools/pulplib/_impl/fake/client.py @@ -277,7 +277,7 @@ def do_next_upload(checksum, size): return out - def _do_unassociate(self, repo_id, type_ids): + def _do_unassociate(self, repo_id, criteria=None): repo_f = self.get_repository(repo_id) if repo_f.exception(): return repo_f @@ -286,8 +286,16 @@ def _do_unassociate(self, repo_id, type_ids): removed = set() kept = set() + # use type hint=Unit so that if type_ids are the goal here + # then we will get back a properly prepared PulpSearch with + # a populated type_ids field + pulp_search = search_for_criteria(criteria, type_hint=Unit, type_ids_accum=None) + for unit in current: - if type_ids is None or unit.content_type_id in type_ids: + if ( + pulp_search.type_ids is None + or unit.content_type_id in pulp_search.type_ids + ): removed.add(unit) else: kept.add(unit) diff --git a/pubtools/pulplib/_impl/model/repository/base.py b/pubtools/pulplib/_impl/model/repository/base.py index 3db36583..31c96797 100644 --- a/pubtools/pulplib/_impl/model/repository/base.py +++ b/pubtools/pulplib/_impl/model/repository/base.py @@ -8,7 +8,7 @@ from ..attr import pulp_attrib from ..distributor import Distributor from ..frozenlist import FrozenList -from ...criteria import Criteria +from ...criteria import Criteria, Matcher from ...schema import load_schema from ... import compat_attr as attr from ...hooks import pm @@ -471,8 +471,34 @@ def remove_content(self, **kwargs): # version of this method will support some "criteria". Let's not fix the # argument order at least until then. - type_ids = kwargs.get("type_ids") - return f_proxy(self._client._do_unassociate(self.id, type_ids)) + # start down the path of using Criteria per this issue: + # https://github.com/release-engineering/pubtools-pulplib/issues/62 + # there seems to be pretty complex handling of Criteria + # for serialization in the search API, and it is unclear which parts + # might also be necessary to use for content removal. + # If any or all of the same handling is needed, it would be beneficial + # to encapsulate the preparation of a criteria JSON object in some + # (more generically named) functions or a class to avoid duplicating code. + # for reference see search_content, _impl.client.Client._search_repo_units, + # _impl.client.Client._search, and _impl.client.search.search_for_criteria + criteria = kwargs.get("criteria", None) + + # prefer criteria keyword argument + # if absent, check for original type_ids keyword arg + if criteria is None: + type_ids = kwargs.get("type_ids") + # use _content_type_id field name to coerce + # search_for_criteria to fill out the PulpSearch#type_ids field + # passing a criteria with an empty type_ids list rather than + # None results in failing tests due to the implementation of + # FakeClient#_do_unassociate + if type_ids is not None: + criteria = Criteria.with_field( + "_content_type_id", + Matcher.in_(type_ids) # Criteria.with_field_in is deprecated + ) + + return f_proxy(self._client._do_unassociate(self.id, criteria=criteria)) @classmethod def from_data(cls, data): From fc1da29f877cba139adaf7317062213b63061e8f Mon Sep 17 00:00:00 2001 From: Ryan Smith Date: Wed, 4 Aug 2021 14:27:58 -0400 Subject: [PATCH 4/5] Remove redundant default of None from call to dict.get --- pubtools/pulplib/_impl/model/repository/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pubtools/pulplib/_impl/model/repository/base.py b/pubtools/pulplib/_impl/model/repository/base.py index 31c96797..01418a4c 100644 --- a/pubtools/pulplib/_impl/model/repository/base.py +++ b/pubtools/pulplib/_impl/model/repository/base.py @@ -481,7 +481,7 @@ def remove_content(self, **kwargs): # (more generically named) functions or a class to avoid duplicating code. # for reference see search_content, _impl.client.Client._search_repo_units, # _impl.client.Client._search, and _impl.client.search.search_for_criteria - criteria = kwargs.get("criteria", None) + criteria = kwargs.get("criteria") # prefer criteria keyword argument # if absent, check for original type_ids keyword arg From dbc3fdd1340827c141fc9c14e293eebb3eafef50 Mon Sep 17 00:00:00 2001 From: Ryan Smith Date: Fri, 6 Aug 2021 08:52:46 -0400 Subject: [PATCH 5/5] Remove support for a criteria keyword arg to remove_content; fix duplicated imports --- pubtools/pulplib/_impl/client/client.py | 10 +------ .../pulplib/_impl/model/repository/base.py | 29 +++++++++---------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/pubtools/pulplib/_impl/client/client.py b/pubtools/pulplib/_impl/client/client.py index 2895c731..57f9b4a1 100644 --- a/pubtools/pulplib/_impl/client/client.py +++ b/pubtools/pulplib/_impl/client/client.py @@ -17,10 +17,6 @@ from .search import search_for_criteria from .errors import PulpException from .poller import TaskPoller -from .search import search_for_criteria -from ..criteria import Criteria -from ..model import Repository, MaintenanceReport, Distributor, Unit -from ..page import Page from . import retry @@ -484,11 +480,7 @@ def _do_unassociate(self, repo_id, criteria=None): # a populated type_ids field pulp_search = search_for_criteria(criteria, type_hint=Unit, type_ids_accum=None) - body = { - "criteria": { - "type_ids": pulp_search.type_ids, - } - } + body = {"criteria": {"type_ids": pulp_search.type_ids}} LOG.debug("Submitting %s unassociate: %s", url, body) diff --git a/pubtools/pulplib/_impl/model/repository/base.py b/pubtools/pulplib/_impl/model/repository/base.py index 01418a4c..5938d1db 100644 --- a/pubtools/pulplib/_impl/model/repository/base.py +++ b/pubtools/pulplib/_impl/model/repository/base.py @@ -473,6 +473,7 @@ def remove_content(self, **kwargs): # start down the path of using Criteria per this issue: # https://github.com/release-engineering/pubtools-pulplib/issues/62 + # by using Criteria internally. # there seems to be pretty complex handling of Criteria # for serialization in the search API, and it is unclear which parts # might also be necessary to use for content removal. @@ -481,22 +482,18 @@ def remove_content(self, **kwargs): # (more generically named) functions or a class to avoid duplicating code. # for reference see search_content, _impl.client.Client._search_repo_units, # _impl.client.Client._search, and _impl.client.search.search_for_criteria - criteria = kwargs.get("criteria") - - # prefer criteria keyword argument - # if absent, check for original type_ids keyword arg - if criteria is None: - type_ids = kwargs.get("type_ids") - # use _content_type_id field name to coerce - # search_for_criteria to fill out the PulpSearch#type_ids field - # passing a criteria with an empty type_ids list rather than - # None results in failing tests due to the implementation of - # FakeClient#_do_unassociate - if type_ids is not None: - criteria = Criteria.with_field( - "_content_type_id", - Matcher.in_(type_ids) # Criteria.with_field_in is deprecated - ) + criteria = None + type_ids = kwargs.get("type_ids") + # use _content_type_id field name to coerce + # search_for_criteria to fill out the PulpSearch#type_ids field. + # passing a criteria with an empty type_ids list rather than + # None results in failing tests due to the implementation of + # FakeClient#_do_unassociate + if type_ids is not None: + criteria = Criteria.with_field( + "_content_type_id", + Matcher.in_(type_ids), # Criteria.with_field_in is deprecated + ) return f_proxy(self._client._do_unassociate(self.id, criteria=criteria))