From 8afb52f65b54cfbd69b551845c81d8dd1c08fc51 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Mar 25 2020 08:34:57 +0000 Subject: Replace image['brew']['build'] with image.nvr Use new property image.nvr to simplify the code. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py b/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py index e49976b..a71c657 100644 --- a/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py +++ b/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py @@ -225,7 +225,7 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): # the ArtifactBuild is created. self.set_context(db_event) - nvr = image["brew"]["build"] + nvr = image.nvr if nvr in builds: self.log_debug("Skipping recording build %s, " "it is already in db", nvr) @@ -239,7 +239,7 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): continue self.log_debug("Recording %s", nvr) - parent_nvr = image["parent"]["brew"]["build"] \ + parent_nvr = image["parent"].nvr \ if "parent" in image and image["parent"] else None dep_on = builds[parent_nvr] if parent_nvr in builds else None @@ -262,7 +262,7 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): state_reason = "" state = ArtifactBuildState.PLANNED.value - image_name = koji.parse_NVR(image["brew"]["build"])["name"] + image_name = koji.parse_NVR(image.nvr)["name"] # Only released images are considered as directly affected for # rebuild. If some image is not in the latest released version and @@ -356,14 +356,15 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): :return: True when image should be filtered out. """ - parsed_nvr = koji.parse_NVR(image["brew"]["build"]) + parsed_nvr = koji.parse_NVR(image.nvr) if not self.event.is_allowed( self, image_name=parsed_nvr["name"], image_version=parsed_nvr["version"], image_release=parsed_nvr["release"]): - self.log_info("Skipping rebuild of image %s, not allowed by " - "configuration", image["brew"]["build"]) + self.log_info( + "Skipping rebuild of image %s, not allowed by configuration", + image.nvr) return True return False diff --git a/freshmaker/image_verifier.py b/freshmaker/image_verifier.py index 4c96208..be4ade3 100644 --- a/freshmaker/image_verifier.py +++ b/freshmaker/image_verifier.py @@ -72,7 +72,7 @@ class ImageVerifier(object): if not image["content_sets"]: raise ValueError( "Found image \"%s\" in this repository, but it cannot be rebuilt, because " - "the \"content_sets\" are not set for this image." % image["brew"]["build"]) + "the \"content_sets\" are not set for this image." % image.nvr) def _get_repository_from_name(self, repo_name): """ @@ -155,7 +155,7 @@ class ImageVerifier(object): self._verify_image_data(image) return { - image["brew"]["build"]: image["content_sets"] + image.nvr: image["content_sets"] } def verify_repository(self, repo_name): @@ -174,9 +174,8 @@ class ImageVerifier(object): images = self.lb.find_images_with_included_srpms( [], [], {repo["repository"]: repo}, include_rpm_manifest=False) for image in images: - nvr = image["brew"]["build"] self._verify_image_data(image) - rebuildable_images[nvr] = image["content_sets"] + rebuildable_images[image.nvr] = image["content_sets"] if not rebuildable_images: raise ValueError( diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index aa039ef..71b8ed7 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -314,7 +314,7 @@ class ContainerImage(dict): non-x86_64 image, OSBS will generate the Pulp repos and therefore we don't need content_sets in Freshmaker. """ - nvr = self["brew"]["build"] + nvr = self.nvr data = {"generate_pulp_repos": False, "content_sets": []} @@ -391,11 +391,10 @@ class ContainerImage(dict): Sets the "repository and "commit" keys/values if available. """ # Find the additional data for Container build in Koji. - nvr = self["brew"]["build"] try: - data = self._get_additional_data_from_koji(nvr) + data = self._get_additional_data_from_koji(self.nvr) except KojiLookupError as e: - err = "Cannot get data from Koji for build %s: %s." % (nvr, e) + err = "Cannot get data from Koji for build %s: %s." % (self.nvr, e) log.error(err) data = self._get_default_additional_data() data["error"] = err @@ -409,9 +408,9 @@ class ContainerImage(dict): :param LightBlue lb_instance: LightBlue instance to use for additional queries. - :param list children: List of children to take the content_sets from in - case this container image is unpublished and therefore without - "content_sets" set. + :param list[ContainerImage] children: List of children to take the + content_sets from in case this container image is unpublished and + therefore without "content_sets" set. """ data = self._get_additional_data_from_distgit( self["repository"], self["git_branch"], self["commit"]) @@ -423,13 +422,13 @@ class ContainerImage(dict): self["content_sets"] = data["content_sets"] self["content_sets_source"] = "distgit" log.info("Container image %s uses following content sets: %r", - self["brew"]["build"], data["content_sets"]) + self.nvr, data["content_sets"]) return # ContainerImage now has content_sets field, so use it if available. if "content_sets" in self and self["content_sets"]: log.info("Container image %s uses following content sets: %r", - self["brew"]["build"], self["content_sets"]) + self.nvr, self["content_sets"]) if "content_sets_source" not in self: self["content_sets_source"] = "lightblue_container_image" return @@ -441,7 +440,7 @@ class ContainerImage(dict): if not children: log.warning("Container image %s does not have 'content_sets' set " "in Lightblue and also does not have any children, " - "this is suspicious.", self["brew"]["build"]) + "this is suspicious.", self.nvr) self.update({"content_sets": []}) return @@ -455,21 +454,21 @@ class ContainerImage(dict): log.info("Container image %s does not have 'content-sets' set " "in Lightblue. Using child image %s content_sets: %r", - self["brew"]["build"], child["brew"]["build"], + self.nvr, child.nvr, child["content_sets"]) self.update({"content_sets": child["content_sets"]}) return log.warning("Container image %s does not have 'content_sets' set " "in Lightblue as well as its children, this " - "is suspicious.", self["brew"]["build"]) + "is suspicious.", self.nvr) self.update({"content_sets": []}) def resolve_published(self, lb_instance): # Get the published version of this image to find out if the image # was actually published. images = lb_instance.get_images_by_nvrs( - [self["brew"]["build"]], published=True, include_rpm_manifest=False) + [self.nvr], published=True, include_rpm_manifest=False) if images: self["published"] = True else: @@ -480,13 +479,11 @@ class ContainerImage(dict): # to check for possible unpublished RPMs. # We do not want to get the complete manifest for every container # image, because it is relatively big, so fetch it only when needed. - images = lb_instance.get_images_by_nvrs( - [self["brew"]["build"]]) + images = lb_instance.get_images_by_nvrs([self.nvr]) if images: self["rpm_manifest"] = images[0]["rpm_manifest"] else: - log.warning("No image %s found in Lightblue.", - self["brew"]["build"]) + log.warning("No image %s found in Lightblue.", self.nvr) def resolve(self, lb_instance, children=None): """ @@ -666,7 +663,7 @@ class LightBlue(object): # TODO: In the future, we may want to combine different ContainerImage # objects into a single object. For now, ensure that whichever object # is used by caller contains multi-arch information. - nvr = image['brew']['build'] + nvr = image.nvr nvr_to_arches.setdefault(nvr, []) nvr_to_arches[nvr].append(image) for arch_image in nvr_to_arches[nvr][:-1]: @@ -826,7 +823,7 @@ class LightBlue(object): # In our case, this means that we filtered out the image. log.info("Will not rebuild %s, because it does not contain " "older version of any input package: %r" % ( - image["brew"]["build"], srpm_name_to_nvrs.values())) + image.nvr, srpm_name_to_nvrs.values())) return ret def filter_out_modularity_mismatch(self, images, srpm_name_to_nvrs): @@ -864,7 +861,7 @@ class LightBlue(object): log.info( "Will not rebuild %s because there is a modularity mismatch between the RPMs " "from the image and the advisory: %r" % ( - image["brew"]["build"], srpm_name_to_nvrs.values())) + image.nvr, srpm_name_to_nvrs.values())) return ret def filter_out_images_based_on_content_set(self, images, content_sets): @@ -989,7 +986,7 @@ class LightBlue(object): # for now. image_nvr_to_image = {} for image in images: - nvr = image["brew"]["build"] + nvr = image.nvr if nvr in image_nvr_to_image: # This image for another architecture has already been seen continue @@ -1268,7 +1265,7 @@ class LightBlue(object): if not latest_parent or "repositories" not in latest_parent: return latest_parent - latest_parent_nvr = kobo.rpmlib.parse_nvr(latest_parent["brew"]["build"]) + latest_parent_nvr = kobo.rpmlib.parse_nvr(latest_parent.nvr) for repo in latest_parent["repositories"]: repo_data = self.get_repository_from_name(repo["repository"]) @@ -1284,13 +1281,13 @@ class LightBlue(object): # - nvr1 newer than nvr2: 1 # - same nvrs: 0 # - nvr1 older: -1 - parsed_nvr = kobo.rpmlib.parse_nvr(possible_latest_parent["brew"]["build"]) + parsed_nvr = kobo.rpmlib.parse_nvr(possible_latest_parent.nvr) if (parsed_nvr["name"] == latest_parent_nvr["name"] and parsed_nvr["version"] == latest_parent_nvr["version"] and kobo.rpmlib.compare_nvr( latest_parent_nvr, parsed_nvr, ignore_epoch=True) == -1): latest_parent = possible_latest_parent - latest_parent_nvr = kobo.rpmlib.parse_nvr(latest_parent["brew"]["build"]) + latest_parent_nvr = kobo.rpmlib.parse_nvr(latest_parent.nvr) return latest_parent @@ -1522,7 +1519,7 @@ class LightBlue(object): # Constructs the temporary dicts as described above. for image_id, images in enumerate(to_rebuild): for parent_id, image in enumerate(images): - nvr = image["brew"]["build"] + nvr = image.nvr # Also include the sorted names of repositories in the image group # to handle the case when different releases of single name-version are # included in different container repositories. @@ -1596,7 +1593,7 @@ class LightBlue(object): if "parent" not in latest_image or not latest_image["parent"]: continue latest_parent_name = koji.parse_NVR( - latest_image["parent"]["brew"]["build"])["name"] + latest_image["parent"].nvr)["name"] # Go through the older images and in case the parent image differs, # update its parents according to latest image parents. @@ -1604,7 +1601,7 @@ class LightBlue(object): image = nvr_to_image[nvr] if "parent" not in image or not image["parent"]: continue - parent_name = koji.parse_NVR(image["parent"]["brew"]["build"])["name"] + parent_name = koji.parse_NVR(image["parent"].nvr)["name"] if parent_name != latest_parent_name: for image_id, parent_id in nvr_to_coordinates[nvr]: latest_image_id, latest_parent_id = nvr_to_coordinates[latest_released_nvr][0] @@ -1666,7 +1663,7 @@ class LightBlue(object): seen = set() for image_rebuild_list in sorted(to_rebuild, key=lambda lst: len(lst), reverse=True): for image, batch in zip(reversed(image_rebuild_list), batches): - image_key = image["brew"]["build"] + image_key = image.nvr # If one of the parents is directly affected but not marked, mark it explicitly if image_key in directly_affected_nvrs and not image.get("directly_affected"): image["directly_affected"] = True @@ -1766,9 +1763,7 @@ class LightBlue(object): # Get all the directly affected images so that any parents that are not marked as # directly affected can be set in _images_to_rebuild_to_batches directly_affected_nvrs = { - image["brew"]["build"] - for image in images - if image.get("directly_affected") + image.nvr for image in images if image.get("directly_affected") } # Now generate batches from deduplicated list and return it. return self._images_to_rebuild_to_batches(to_rebuild, directly_affected_nvrs) diff --git a/tests/handlers/koji/test_rebuild_images_on_rpm_advisory_change.py b/tests/handlers/koji/test_rebuild_images_on_rpm_advisory_change.py index 237e5de..7ab521c 100644 --- a/tests/handlers/koji/test_rebuild_images_on_rpm_advisory_change.py +++ b/tests/handlers/koji/test_rebuild_images_on_rpm_advisory_change.py @@ -681,19 +681,19 @@ class TestAllowBuild(helpers.ModelsTestCase): security_impact="None", product_short_name="product")) - image = {"brew": {"build": "foo-1-2.3"}} + image = ContainerImage({"brew": {"build": "foo-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "foo2-1-2.3"}} + image = ContainerImage({"brew": {"build": "foo2-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "bar-1-2.3"}} + image = ContainerImage({"brew": {"build": "bar-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "unknown-1-2.3"}} + image = ContainerImage({"brew": {"build": "unknown-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, True) @@ -721,11 +721,11 @@ class TestAllowBuild(helpers.ModelsTestCase): security_impact="None", product_short_name="product")) - image = {"brew": {"build": "foo-1-2.3"}} + image = ContainerImage({"brew": {"build": "foo-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "unknown-1-2.3"}} + image = ContainerImage({"brew": {"build": "unknown-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, True) @@ -761,19 +761,19 @@ class TestAllowBuild(helpers.ModelsTestCase): security_impact="None", product_short_name="product")) - image = {"brew": {"build": "foo-1-2.3"}} + image = ContainerImage({"brew": {"build": "foo-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "foo-1-7.3"}} + image = ContainerImage({"brew": {"build": "foo-1-7.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, False) - image = {"brew": {"build": "foo-7.3-2.3"}} + image = ContainerImage({"brew": {"build": "foo-7.3-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, True) - image = {"brew": {"build": "unknown-1-2.3"}} + image = ContainerImage({"brew": {"build": "unknown-1-2.3"}}) ret = handler._filter_out_not_allowed_builds(image) self.assertEqual(ret, True) @@ -793,7 +793,7 @@ class TestBatches(helpers.ModelsTestCase): def _mock_build( self, build, parent=None, error=None, **kwargs): if parent: - parent = {"brew": {"build": parent + "-1-1.25"}} + parent = ContainerImage({"brew": {"build": parent + "-1-1.25"}}) d = { 'brew': {'build': build + "-1-1.25"}, 'repository': build + '_repo', @@ -856,7 +856,7 @@ class TestBatches(helpers.ModelsTestCase): images = {} for batch in batches: for image in batch: - images[image['brew']['build']] = image + images[image.nvr] = image # Record the batches. event = events.BrewSignRPMEvent("123", "openssl-1.1.0-1") diff --git a/tests/test_image_verifier.py b/tests/test_image_verifier.py index 9fa13cc..2e93a2d 100644 --- a/tests/test_image_verifier.py +++ b/tests/test_image_verifier.py @@ -24,6 +24,7 @@ from unittest.mock import MagicMock from freshmaker.image_verifier import ImageVerifier +from freshmaker.lightblue import ContainerRepository, ContainerImage from tests import helpers @@ -85,39 +86,57 @@ class TestImageVerifier(helpers.FreshmakerTestCase): self.verifier.verify_repository, "foo/bar") def test_verify_repository_no_content_sets(self): - self.lb.find_container_repositories.return_value = [{ - "repository": "foo/bar", - "release_categories": ["Generally Available"], - "published": True, - "auto_rebuild_tags": ["latest"]}] - self.lb.find_images_with_included_srpms.return_value = [{ - "brew": {"build": "foo-1-1"}, - "content_sets": []}] + self.lb.find_container_repositories.return_value = [ + ContainerRepository({ + "repository": "foo/bar", + "release_categories": ["Generally Available"], + "published": True, + "auto_rebuild_tags": ["latest"] + }) + ] + self.lb.find_images_with_included_srpms.return_value = [ + ContainerImage({ + "brew": {"build": "foo-1-1"}, + "content_sets": [] + }) + ] self.assertRaisesRegex( ValueError, r'.*are not set for this image.', self.verifier.verify_repository, "foo/bar") def test_verify_repository(self): - self.lb.find_container_repositories.return_value = [{ - "repository": "foo/bar", - "release_categories": ["Generally Available"], - "published": True, - "auto_rebuild_tags": ["latest"]}] - self.lb.find_images_with_included_srpms.return_value = [{ - "brew": {"build": "foo-1-1"}, - "content_sets": ["content-set"]}] + self.lb.find_container_repositories.return_value = [ + ContainerRepository({ + "repository": "foo/bar", + "release_categories": ["Generally Available"], + "published": True, + "auto_rebuild_tags": ["latest"] + }) + ] + self.lb.find_images_with_included_srpms.return_value = [ + ContainerImage({ + "brew": {"build": "foo-1-1"}, + "content_sets": ["content-set"] + }) + ] ret = self.verifier.verify_repository("foo/bar") self.assertEqual(ret, {"foo-1-1": ["content-set"]}) def test_get_verify_image(self): - self.lb.find_container_repositories.return_value = [{ - "repository": "foo/bar", - "release_categories": ["Generally Available"], - "published": True, - "auto_rebuild_tags": ["latest"]}] - self.lb.get_images_by_nvrs.return_value = [{ - "brew": {"build": "foo-1-1"}, - "content_sets": ["content-set"]}] + self.lb.find_container_repositories.return_value = [ + ContainerRepository({ + "repository": "foo/bar", + "release_categories": ["Generally Available"], + "published": True, + "auto_rebuild_tags": ["latest"] + }) + ] + self.lb.get_images_by_nvrs.return_value = [ + ContainerImage({ + "brew": {"build": "foo-1-1"}, + "content_sets": ["content-set"] + }) + ] ret = self.verifier.verify_image("foo-1-1") self.assertEqual(ret, {"foo-1-1": ["content-set"]}) diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 5886fa6..07fc0c3 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -1301,7 +1301,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ["dummy-content-set-1", "dummy-content-set-2"], ["openssl-1.2.3-2"], repositories) self.assertEqual( - [image["brew"]["build"] for image in ret], + [image.nvr for image in ret], ['package-name-2-4-12.10', 'package-name-3-4-12.10']) @patch('freshmaker.lightblue.LightBlue.find_container_images') @@ -1342,11 +1342,11 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ["dummy-content-set-1", "dummy-content-set-2"], ["openssl-1.2.3-1", "openssl-1.2.3-50"], repositories) self.assertEqual( - [image["brew"]["build"] for image in ret], + [image.nvr for image in ret], ['package-name-2-4-12.10', 'package-name-3-4-12.10']) def _filter_fnc(self, image): - return image["brew"]["build"].startswith("filtered_") + return image.nvr.startswith("filtered_") @patch('freshmaker.lightblue.LightBlue.find_container_repositories') @patch('freshmaker.lightblue.LightBlue.find_container_images') @@ -1816,12 +1816,12 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): [leaf_image3] ] expected_batches_nvrs = [ - {image["brew"]["build"] for image in batch} + {image.nvr for image in batch} for batch in expected_batches ] returned_batches_nvrs = [ - {image["brew"]["build"] for image in batch} + {image.nvr for image in batch} for batch in batches ] @@ -1831,7 +1831,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): # find_images_with_packages_from_content_set. for batch in batches: for image in batch: - if image["brew"]["build"] == "leaf-image-6-1": + if image.nvr == "leaf-image-6-1": self.assertTrue(leaf_image6_as_parent["directly_affected"]) break @@ -2070,7 +2070,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): cert=self.fake_cert_file, private_key=self.fake_private_key) image = lb.find_latest_parent_image("foo", 1) - self.assertEqual(image["brew"]["build"], "parent-1-3") + self.assertEqual(image.nvr, "parent-1-3") @patch('freshmaker.lightblue.LightBlue.find_container_images') @patch('os.path.exists') @@ -2089,12 +2089,12 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ret = lb.find_images_with_included_srpms( ["dummy-content-set-1", "dummy-content-set-2"], ["openssl-1.2.3-2.module+el8.0.0+3248+9d514f3b.src"], repositories) self.assertEqual( - [image["brew"]["build"] for image in ret], + [image.nvr for image in ret], ["package-name-3-4-12.10"]) ret = lb.find_images_with_included_srpms( ["dummy-content-set-1", "dummy-content-set-2"], ["openssl-1.2.3-2.el8.0.0+3248+9d514f3b.src"], repositories) self.assertEqual( - [image["brew"]["build"] for image in ret], + [image.nvr for image in ret], []) @patch('freshmaker.lightblue.LightBlue.find_container_images') @@ -2126,7 +2126,7 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): ret = lb.find_images_with_included_srpms( ["dummy-content-set-1"], ["openssl-1.2.3-2.module+el8.0.0+3248+9d514f3b.src"], repositories) self.assertEqual( - [image["brew"]["build"] for image in ret], + [image.nvr for image in ret], ["parent-1-2", "parent-1-3"]) @@ -2485,9 +2485,7 @@ class TestDeduplicateImagesToRebuild(helpers.FreshmakerTestCase): ]) to_rebuild = [httpd, perl] batches = self.lb._images_to_rebuild_to_batches(to_rebuild, set()) - batches = [ - sorted_by_nvr(images, get_nvr=lambda image: image['brew']['build']) - for images in batches] + batches = [sorted_by_nvr(images) for images in batches] # Both perl and httpd share the same parent images, so include # just httpd's one in expected batches - they are the same as @@ -2521,16 +2519,14 @@ class TestDeduplicateImagesToRebuild(helpers.FreshmakerTestCase): to_rebuild.append(deps[i:]) batches = self.lb._images_to_rebuild_to_batches(to_rebuild, {httpd_nvr}) - batches = [ - sorted_by_nvr(images, get_nvr=lambda image: image['brew']['build']) - for images in batches] + batches = [sorted_by_nvr(images) for images in batches] # We expect each image to be rebuilt just once. expected = [[deps[3]], [deps[2]], [deps[1]], [deps[0]]] self.assertEqual(batches, expected) for batch in batches: for image in batch: - if image["brew"]["build"] == httpd_nvr: + if image.nvr == httpd_nvr: self.assertTrue(image['directly_affected']) else: self.assertFalse(image.get('directly_affected'))