From d9b0a41e1c77b2649db64bb36ec636634fad6f4f Mon Sep 17 00:00:00 2001 From: Robert Bikar Date: Wed, 25 Sep 2024 13:02:02 +0200 Subject: [PATCH] Added support for Distributor when creating repository Previously it wasn't possible to create repository with distributor because of inconsintent naming of various fields which resulted in Bad requests. This is now fixed and distributors can be created along repository. Only mininal configuration is currently supported. --- src/pubtools/pulplib/_impl/client/client.py | 87 +++++++++++++++++---- tests/fake/test_fake_create_repo.py | 23 +++++- tests/repository/test_yum_repository.py | 85 ++++++++++++++++++-- 3 files changed, 172 insertions(+), 23 deletions(-) diff --git a/src/pubtools/pulplib/_impl/client/client.py b/src/pubtools/pulplib/_impl/client/client.py index 2e3f13ff..ba0732a6 100644 --- a/src/pubtools/pulplib/_impl/client/client.py +++ b/src/pubtools/pulplib/_impl/client/client.py @@ -10,7 +10,7 @@ from more_executors import Executors from more_executors.futures import f_map, f_flat_map, f_return, f_proxy, f_sequence from io import StringIO - +from ..compat_attr import evolve from ..model.repository.repo_lock import LOCK_CLAIM_STR from ..page import Page from ..criteria import Criteria @@ -1083,6 +1083,13 @@ def create_repository(self, repo): body["importer_type_id"] = importer[0]["importer_type_id"] if importer else None body["importer_config"] = importer[0]["config"] if importer else None + # re-key distributor dict keys due to pulp API inconsistency + for item in body["distributors"]: + item["distributor_id"] = item["id"] + item["distributor_config"] = item["config"] + del item["config"] + del item["id"] + def log_existing_repo(exception): if ( getattr(exception, "response", None) is not None @@ -1094,25 +1101,58 @@ def log_existing_repo(exception): raise exception def check_repo(repo_on_server): + # evolve some fields that don't have to be set before repository creation + # but they're typically set automatically after creation call which results in + # inequality between `repo_on_server` and `repo` + dists = [evolve(item, repo_id=repo_id) for item in repo.distributors] + repo_updated = evolve( + repo, relative_url=repo_on_server.relative_url, distributors=dists + ) try: assert ( - repo_on_server == repo + repo_on_server == repo_updated ), "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, + self._check_resource( + "importer", + ["type_id", "config"], + repo_updated.importer, + repo_on_server.importer, + repo_updated.id, + ) + + extra_dist = set() + missing_dist = set() + for expected_item in repo_updated.distributors: + for current_item in repo_on_server.distributors: + if expected_item.type_id == current_item.type_id: + self._check_resource( + "distributor", + ["id", "type_id", "last_publish"], + expected_item, + current_item, + repo_updated.id, ) + extra_dist.discard(current_item.type_id) + missing_dist.discard(expected_item.type_id) + else: + extra_dist.add(current_item.type_id) + missing_dist.add(expected_item.type_id) + + for item in extra_dist: + LOG.error( + "Repository %s contains unexpected distributor with type: %s", + repo_id, + item, + ) + for item in missing_dist: + LOG.error( + "Repository %s is missing distributor with type: %s", + repo_id, + item, + ) + LOG.exception( "Repository %s exists on server and contains unexpected values", repo_id, @@ -1131,3 +1171,22 @@ def check_repo(repo_on_server): out = f_flat_map(out, check_repo) return f_proxy(out) + + @staticmethod + def _check_resource( + resource_type, fields, expected_resource, current_resource, repo_id + ): + for attr in fields: + expected = getattr(expected_resource, attr) + current = getattr(current_resource, attr) + if expected != current: + LOG.error( + "Repository %s contains wrong %s %s\n" + "\t expected: %s\n" + "\t current: %s\n", + repo_id, + resource_type, + attr, + expected, + current, + ) diff --git a/tests/fake/test_fake_create_repo.py b/tests/fake/test_fake_create_repo.py index 06363e69..7d8d5978 100644 --- a/tests/fake/test_fake_create_repo.py +++ b/tests/fake/test_fake_create_repo.py @@ -1,4 +1,4 @@ -from pubtools.pulplib import FakeController, Repository +from pubtools.pulplib import FakeController, Repository, Distributor def test_create_repository(): @@ -6,11 +6,26 @@ def test_create_repository(): controller = FakeController() client = controller.client - repo_1 = client.create_repository(Repository(id="repo1")) - repo_2 = client.create_repository(Repository(id="repo2")) + repo_1 = client.create_repository( + Repository( + id="repo1", + distributors=[Distributor(id="dist1", type_id="yum_distributor")], + ) + ) + repo_2 = client.create_repository( + Repository( + id="repo2", + distributors=[Distributor(id="dist2", type_id="yum_distributor")], + ) + ) # adding already existing repository has no effect - _ = client.create_repository(Repository(id="repo1")) + _ = client.create_repository( + Repository( + id="repo1", + distributors=[Distributor(id="dist1", type_id="yum_distributor")], + ) + ) # The change should be reflected in the controller, # with two repositories present assert controller.repositories == [repo_1.result(), repo_2.result()] diff --git a/tests/repository/test_yum_repository.py b/tests/repository/test_yum_repository.py index b082139a..8b12f80f 100644 --- a/tests/repository/test_yum_repository.py +++ b/tests/repository/test_yum_repository.py @@ -2,7 +2,14 @@ import logging import requests -from pubtools.pulplib import Repository, YumRepository, DetachedException, YumImporter +import pubtools.pulplib._impl.compat_attr as attr +from pubtools.pulplib import ( + Repository, + YumRepository, + DetachedException, + YumImporter, + Distributor, +) def test_from_data_gives_yum_repository(): @@ -216,7 +223,16 @@ def test_related_repositories_detached_client(): def test_create_repository(client, requests_mocker): - repo = YumRepository(id="yum_repo_new") + repo = YumRepository( + id="yum_repo_new", + distributors=[ + Distributor( + id="fake_id", + type_id="yum_distributor", + relative_url="yum_repo_new/foo/bar", + ) + ], + ) # create request requests_mocker.post( @@ -237,13 +253,44 @@ def test_create_repository(client, requests_mocker): "config": {}, } ], + "distributors": [ + { + "id": "fake_id", + "distributor_type_id": "yum_distributor", + "config": {"relative_url": "yum_repo_new/foo/bar"}, + "repo_id": "yum_repo_new", + } + ], } ], ) - out = client.create_repository(repo) + out = client.create_repository(repo).result() # check return value of create_repository() call - assert out.result() == repo + # relative_url wasn't set before request for `repo` + assert repo.relative_url is None + # but it's set after creation + assert out.relative_url == "yum_repo_new/foo/bar" + + # repo_id of distributor isn't set before creation call + assert repo.distributors[0].repo_id is None + # but it's set after creation + assert out.distributors[0].repo_id == "yum_repo_new" + # evolve original repo object with new values + repo = attr.evolve( + repo, + relative_url="yum_repo_new/foo/bar", + distributors=[ + Distributor( + id="fake_id", + type_id="yum_distributor", + relative_url="yum_repo_new/foo/bar", + repo_id="yum_repo_new", + ) + ], + ) + # and do full assert + assert out == repo hist = requests_mocker.request_history # there should be exactly 2 requests sent - create and search @@ -256,6 +303,14 @@ def test_create_repository(client, requests_mocker): # are automatically added for yum repository assert create_query["importer_type_id"] == "yum_importer" assert create_query["importer_config"] == {} + # check distributor data + assert len(create_query["distributors"]) == 1 + assert create_query["distributors"][0]["distributor_id"] == "fake_id" + assert create_query["distributors"][0]["distributor_type_id"] == "yum_distributor" + assert ( + create_query["distributors"][0]["distributor_config"]["relative_url"] + == "yum_repo_new/foo/bar" + ) # check the search request for created repo search_query = hist[1].json() @@ -331,7 +386,12 @@ def test_create_repository_already_exists(client, requests_mocker, caplog): def test_create_repository_wrong_data(client, requests_mocker, caplog): repo = YumRepository( - id="yum_repo_existing", importer=YumImporter(config={"new": "value"}) + id="yum_repo_existing", + importer=YumImporter(config={"new": "value"}), + distributors=[ + Distributor(id="test_dist_1", type_id="yum_distributor"), + Distributor(id="test_dist_2", type_id="rpm_rsync_distributor"), + ], ) requests_mocker.post( @@ -353,6 +413,18 @@ def test_create_repository_wrong_data(client, requests_mocker, caplog): "config": {"current": "value"}, } ], + "distributors": [ + { + "id": "fake_id_1", + "distributor_type_id": "yum_distributor", + "config": {}, + }, + { + "id": "fake_id_2", + "distributor_type_id": "iso_distributor", + "config": {}, + }, + ], } ], ) @@ -363,6 +435,9 @@ def test_create_repository_wrong_data(client, requests_mocker, caplog): for text in ( "Repository yum_repo_existing already exists", "Repository yum_repo_existing contains wrong importer config", + "Repository yum_repo_existing contains wrong distributor id", + "Repository yum_repo_existing contains unexpected distributor with type: iso_distributor", + "Repository yum_repo_existing is missing distributor with type: rpm_rsync_distributor", "Repository yum_repo_existing exists on server and contains unexpected values", ): assert text in caplog.text