Review fixes #1

Changes:
* Client.create_repository() now returns fully usable repository that
  was created with client attached
* Also the create_repository() checks the created repository and reports
  if there is some discrepancy between requested repo and
  created/existing repo
* IsoImporter -> FileImporter
* fixed and updated tests
* added `"importers": True` to search options for repos
* fixed converter of `notes.signatures` on `Repository` - empty string is now
  converted to empty list, not to list with empty string

...this commit will be squashed

Author: rbikar

docs/api/files.rst

@@ -14,7 +14,7 @@ Repository
 .. autoclass:: pubtools.pulplib.FileSyncOptions()
    :members:
 
-.. autoclass:: pubtools.pulplib.IsoImporter()
+.. autoclass:: pubtools.pulplib.FileImporter()
    :members:
 
 Units

pubtools/pulplib/__init__.py

@@ -33,6 +33,6 @@
     MaintenanceEntry,
     Importer,
     YumImporter,
-    IsoImporter,
+    FileImporter,
 )
 from ._impl.fake import FakeController

pubtools/pulplib/_impl/client/client.py

@@ -278,7 +278,7 @@ def search_repository(self, criteria=None):
                 Each page will contain a collection of
                 :class:`~pubtools.pulplib.Repository` objects.
         """
-        search_options = {"distributors": True}
+        search_options = {"distributors": True, "importers": True}
         return self._search(
             Repository, "repositories", criteria=criteria, search_options=search_options
         )
@@ -1058,15 +1058,18 @@ def _do_sync(self, repo_id, sync_options):
     def create_repository(self, repo):
         """Create a repository with initial data provided in the
            argument. Importer for repository is automatically associated
-           if available. If repository already exists, only warning is logged.
+           if available. If repository already exists, warning is logged.
+           After repository has been successfully created or if repository already exists,
+           it is checked if expected and current repository configuration values are
+           correct.
 
         Args:
             repo (:class:`~pubtools.pulplib.Repository`)
                 A repository object used for creation.
 
         Returns:
-            Future
-                A future which is resolved with a value of ``None`` once the
+            Future[Repository]
+                A future which is resolved with a value of ``Repository`` once the
                 repository has been created.
 
         .. versionadded:: 2.39.0
@@ -1090,10 +1093,41 @@ def log_existing_repo(exception):
 
             raise exception
 
+        def get_and_check_repo(_):
+            repo_on_server = self.get_repository(repo_id)
+            try:
+                assert (
+                    repo_on_server.result() == repo
+                ), "Repo exists on server with unexpected values"
+            except AssertionError:
+                if importer:
+                    for attr in ["type_id", "config"]:
+                        expected = getattr(repo.importer, attr)
+                        current = getattr(repo_on_server.importer, attr)
+                        if expected != current:
+                            LOG.error(
+                                "Repository %s contains wrong importer %s\n"
+                                "\t expected: %s\n"
+                                "\t current: %s\n",
+                                repo_id,
+                                attr,
+                                expected,
+                                current,
+                            )
+                LOG.exception(
+                    "Repository %s exists on server and contains unexpected values",
+                    repo_id,
+                )
+                raise
+
+            return repo_on_server
+
         LOG.debug("Creating repository %s", repo_id)
         out = self._request_executor.submit(
             self._do_request, method="POST", url=url, json=body
         )
 
         out = f_map(out, error_fn=log_existing_repo)
-        return f_map(out, lambda _: None)
+        out = f_flat_map(out, get_and_check_repo)
+
+        return f_proxy(out)

pubtools/pulplib/_impl/fake/client.py

@@ -659,4 +659,4 @@ def create_repository(self, repo):
             ]:
                 self._state.repositories.append(repo)
 
-        return f_return(None)
+        return self.get_repository(repo.id)

pubtools/pulplib/_impl/model/__init__.py

@@ -10,7 +10,7 @@
     ContainerSyncOptions,
     YumSyncOptions,
     Importer,
-    IsoImporter,
+    FileImporter,
     YumImporter,
 )
 from .unit import (

pubtools/pulplib/_impl/model/repository/__init__.py

@@ -1,4 +1,4 @@
 from .base import Repository, PublishOptions, SyncOptions, Importer
 from .container import ContainerImageRepository, ContainerSyncOptions
 from .yum import YumRepository, YumSyncOptions, YumImporter
-from .file import FileRepository, FileSyncOptions, IsoImporter
+from .file import FileRepository, FileSyncOptions, FileImporter

pubtools/pulplib/_impl/model/repository/base.py

@@ -8,10 +8,11 @@
 from frozenlist2 import frozenlist
 from more_executors.futures import f_proxy, f_map, f_flat_map
 
+from frozendict.core import frozendict  # pylint: disable=no-name-in-module
 from .repo_lock import RepoLock
 from ..attr import pulp_attrib, PULP2_FIELD, PULP2_MUTABLE
 from ..common import PulpObject, Deletable, DetachedException
-from ..convert import frozenlist_or_none_converter
+from ..convert import frozenlist_or_none_converter, frozendict_or_none_converter
 from ..distributor import Distributor
 from ...criteria import Criteria, Matcher
 from ...schema import load_schema
@@ -47,7 +48,12 @@ class Importer(PulpObject):
     """
     Type id of the importer.
     """
-    config = pulp_attrib(default=None, type=dict, pulp_field="config")
+    config = pulp_attrib(
+        default=attr.Factory(frozendict),
+        type=frozendict,
+        converter=frozendict_or_none_converter,
+        pulp_field="config",
+    )
     """
     Configuration dictionary of the importer.
     """
@@ -283,7 +289,7 @@ class Repository(PulpObject, Deletable):
         default=attr.Factory(frozenlist),
         type=list,
         pulp_field="notes.signatures",
-        pulp_py_converter=lambda sigs: sigs.split(","),
+        pulp_py_converter=lambda sigs: sigs.split(",") if sigs else [],
         py_pulp_converter=",".join,
         converter=lambda keys: frozenlist([k.strip() for k in keys]),
     )
@@ -383,7 +389,7 @@ class Repository(PulpObject, Deletable):
     """
     An object of :class:`~pubtools.pulplib.Importer` that is associated with the repository.
 
-    .. versionadded:: 2.39.0 
+    .. versionadded:: 2.39.0
     """
 
     @distributors.validator

pubtools/pulplib/_impl/model/repository/file.py

@@ -14,7 +14,7 @@
 
 
 @attr.s(kw_only=True, frozen=True)
-class IsoImporter(Importer):
+class FileImporter(Importer):
     type_id = pulp_attrib(
         default="iso_importer", type=str, pulp_field="importer_type_id"
     )
@@ -60,14 +60,14 @@ class FileRepository(Repository):
     )
 
     importer = pulp_attrib(
-        default=IsoImporter(),
-        type=IsoImporter,
+        default=FileImporter(),
+        type=FileImporter,
         pulp_field="importers",
-        pulp_py_converter=IsoImporter._from_data,
-        py_pulp_converter=IsoImporter._to_data,
+        pulp_py_converter=FileImporter._from_data,
+        py_pulp_converter=FileImporter._to_data,
     )
     """
-    An object of :class:`~pubtools.pulplib.IsoImporter` that is associated with the repository.
+    An object of :class:`~pubtools.pulplib.FileImporter` that is associated with the repository.
 
     .. versionadded:: 2.39.0 
     """

pubtools/pulplib/_impl/model/repository/yum.py

@@ -123,7 +123,7 @@ class YumRepository(Repository):
     """
     An object of :class:`~pubtools.pulplib.YumImporter` that is associated with the repository.
 
-    .. versionadded:: 2.39.0 
+    .. versionadded:: 2.39.0
     """
 
     def get_binary_repository(self):

tests/fake/test_fake_create_repo.py

@@ -6,11 +6,11 @@ def test_create_repository():
     controller = FakeController()
 
     client = controller.client
-    client.create_repository(Repository(id="repo1"))
-    client.create_repository(Repository(id="repo2"))
+    repo_1 = client.create_repository(Repository(id="repo1"))
+    repo_2 = client.create_repository(Repository(id="repo2"))
 
     # adding already existing repository has no effect
-    client.create_repository(Repository(id="repo1"))
+    _ = client.create_repository(Repository(id="repo1"))
     # The change should be reflected in the controller,
     # with two repositories present
-    assert controller.repositories == [Repository(id="repo1"), Repository(id="repo2")]
+    assert controller.repositories == [repo_1.result(), repo_2.result()]