PR#495: Refactor find latest image by its build NV - freshmaker

freshmaker

#495 Refactor find latest image by its build NV

Merged 4 years ago by cqi. Opened 4 years ago by cqi.

cqi/freshmaker refactor into master

Refactor find latest image by its build NV

Chenxiong Qi • 4 years ago

d224861

freshmaker/lightblue.py

file modified

+22 -23

		`@@ -147,7 +147,11 @@`
		`return image`

		`def __hash__(self):`
		`- return hash((self['brew']['build']))`
		`+ return hash((self.nvr))`
		`+`
		`+ @property`
		`+ def nvr(self):`
		`+ return self['brew']['build']`

		`def log_error(self, err):`
		`"""`
		`@@ -157,7 +161,7 @@`
		`"""`
		`prefix = ""`
		`if 'brew' in self and 'build' in self['brew']:`
		`- prefix = self['brew']['build'] + ": "`
		`+ prefix = self.nvr + ": "`
		`log.error("%s%s", prefix, err)`
		`if 'error' not in self or not self['error']:`
		`self['error'] = str(err)`
		`@@ -191,8 +195,7 @@`
		`dockerfile = [file for file in self['parsed_data']['files']`
		`if file['filename'] == 'Dockerfile']`
		`if not dockerfile:`
		`- log.warning('Image %s does not contain a Dockerfile.',`
		`- self['brew']['build'])`
		`+ log.warning('Image %s does not contain a Dockerfile.', self.nvr)`
		`return None`
		`return dockerfile[0]`

		`@@ -885,7 +888,7 @@`
		`ret = []`
		`for image in images:`
		`if not content_sets & set(image["content_sets"]):`
		`- log.info(f"Will not rebuild {image['brew']['build']} because its content_sets "`
		`+ log.info(f"Will not rebuild {image.nvr} because its content_sets "`
		`"({image['content_sets']}) are not related to the requested content_sets"`
		`" ({content_sets})")`
		`else:`
		`@@ -1406,6 +1409,7 @@`
		`the given image - can be used for comparisons if needed`
		`:rtype: list`
		`"""`
		`+`
		`repos = self.find_all_container_repositories(published, release_categories)`
		`if not repos:`
		`return []`
		`@@ -1422,30 +1426,26 @@`
		`# architecture, it is only interested in NVR, so group the images`
		`# by the same image['brew']['build'] and include just first one in the`
		`# image list.`
		`- sorted_images = sorted_by_nvr(`
		`- images, get_nvr=lambda image: image['brew']['build'], reverse=True)`
		`+ sorted_images = sorted_by_nvr(images, reverse=True)`
		`images = []`
		`- for k, v in groupby(sorted_images, key=lambda x: x['brew']['build']):`
		`+ for k, v in groupby(sorted_images, key=lambda item: item.nvr):`
		`images.append(next(v))`

		`# In case we query for unpublished images, we need to return just`
		`# the latest NVR for given name-version, otherwise images would`
		`# contain all the versions which ever containing the srpm_name.`
		`if not published:`
		`- # Sort images by brew build NVR descending`
		`- sorted_images = sorted_by_nvr(`
		`- images, get_nvr=lambda image: image['brew']['build'], reverse=True)`

		`- # Iterate over all the images and only keep the very first one`
		`- # with the given name-version - this is the latest one.`
		`- images = []`
		`- seen_name_versions = []`
		`- for image in sorted_images:`
		`- parsed_build = koji.parse_NVR(image["brew"]["build"])`
		`- nv = "%s-%s" % (parsed_build["name"], parsed_build["version"])`
		`- if nv not in seen_name_versions:`
		`- images.append(image)`
		`- seen_name_versions.append(nv)`
		`+ def _name_version_key(item):`
		`+ nvr = koji.parse_NVR(item.nvr)`
		`+ return f"{nvr['name']}-{nvr['version']}"`
		`+`
		`+ images = [`
		`+ next(grouped_images) for _, grouped_images in groupby(`
		`+ sorted_by_nvr(images, reverse=True),`
		`+ key=_name_version_key`
		`+ )`
		`+ ]`

		`# Filter out images based on the filter_fnc.`
		`if filter_fnc:`
		`@@ -1726,8 +1726,7 @@`
		# This `srpm_name` is not in image.
		`continue`

		`- unpublished = self.find_unpublished_image_for_build(`
		`- image['brew']['build'])`
		`+ unpublished = self.find_unpublished_image_for_build(image.nvr)`
		`if not unpublished:`
		`image.log_error(`
		`"Cannot find unpublished version of image, Lightblue "`

freshmaker/utils.py

file modified

		`@@ -59,6 +59,9 @@`
		`if get_nvr:`
		`nvr1 = get_nvr(item1)`
		`nvr2 = get_nvr(item2)`
		`+ elif hasattr(item1, 'nvr') and hasattr(item2, 'nvr'):`
		`+ nvr1 = item1.nvr`
		`+ nvr2 = item2.nvr`
		`else:`
		`nvr1 = item1`
		`nvr2 = item2`

tests/test_lightblue.py

file modified

+105

		`@@ -1446,6 +1446,111 @@`
		`@patch('freshmaker.kojiservice.KojiService.get_build')`
		`@patch('freshmaker.kojiservice.KojiService.get_task_request')`
		`@patch('os.path.exists')`
		`+ def test_images_with_content_set_packages_unpublished(`
		`+ self, exists, koji_task_request, koji_get_build, cont_images, cont_repos`
		`+ ):`
		`+ exists.return_value = True`
		`+ cont_repos.return_value = self.fake_repositories_with_content_sets`
		`+`
		`+ # "filtered_x-1-23" image will be filtered by filter_fnc.`
		`+ cont_images.return_value = self.fake_container_images + [`
		`+ ContainerImage.create(`
		`+ {`
		`+ "content_sets": ["dummy-content-set-1"],`
		`+ "brew": {"build": "filtered_x-1-23"},`
		`+ "repositories": [`
		`+ {`
		`+ "repository": "product/repo1",`
		`+ "published": True,`
		`+ "tags": [{"name": "latest"}]`
		`+ }`
		`+ ]`
		`+ }`
		`+ )]`
		`+ # Include the images for second time to ensure that they will be`
		`+ # returned only once. This can happen when the image is multiarch.`
		`+ cont_images.return_value += self.fake_container_images`
		`+`
		`+ koji_task_request.side_effect = self.fake_koji_task_requests`
		`+ koji_get_build.side_effect = self.fake_koji_builds`
		`+`
		`+ lb = LightBlue(server_url=self.fake_server_url,`
		`+ cert=self.fake_cert_file,`
		`+ private_key=self.fake_private_key)`
		`+`
		`+ ret = lb.find_images_with_packages_from_content_set(`
		`+ set(["openssl-1.2.3-3"]), ["dummy-content-set-1"],`
		`+ filter_fnc=self._filter_fnc,`
		`+ published=False`
		`+ )`
		`+`
		`+ # Only the first image should be returned, because the first one`
		`+ # is in repository "product1/repo1", but we have asked for images`
		`+ # in repository "product/repo1".`
		`+ self.assertEqual(1, len(ret))`
		`+ self.assertEqual(ret,`
		`+ [`
		`+ {`
		`+ "latest_released": True,`
		`+ "generate_pulp_repos": True,`
		`+ "repository": "rpms/repo-2",`
		`+ "commit": "commit_hash2",`
		`+ "target": "target2",`
		`+ "git_branch": "mybranch",`
		`+ "error": None,`
		`+ "arches": None,`
		`+ "odcs_compose_ids": None,`
		`+ "parent_build_id": None,`
		`+ "parent_image_builds": None,`
		`+ "multi_arch_rpm_manifest": {},`
		`+ "published": True,`
		`+ "brew": {`
		`+ "completion_date": u"20170421T04:27:51.000-0400",`
		`+ "build": "package-name-2-4-12.10",`
		`+ "package": "package-name-2"`
		`+ },`
		`+ 'content_sets': ["dummy-content-set-1"],`
		`+ 'content_sets_source': 'lightblue_container_image',`
		`+ 'directly_affected': True,`
		`+ "release_categories": ["Generally Available"],`
		`+ 'repositories': [`
		`+ {'repository': 'product2/repo2', 'published': True,`
		`+ 'tags': [{"name": "latest"}]}`
		`+ ],`
		`+ 'parsed_data': {`
		`+ 'files': [`
		`+ {`
		`+ 'key': 'buildfile',`
		`+ 'content_url': 'http://git.repo.com/cgit/rpms/repo-2/plain/Dockerfile?id=commit_hash2',`
		`+ 'filename': 'Dockerfile'`
		`+ },`
		`+ {`
		`+ 'key': 'bogusfile',`
		`+ 'content_url': 'bogus_test_url',`
		`+ 'filename': 'bogus.file'`
		`+ }`
		`+ ]`
		`+ },`
		`+ 'rpm_manifest': [{`
		`+ 'rpms': [`
		`+ {`
		`+ "srpm_name": "openssl",`
		`+ "srpm_nevra": "openssl-1:1.2.3-1.src"`
		`+ },`
		`+ {`
		`+ "srpm_name": "tespackage2",`
		`+ "srpm_nevra": "testpackage2-10:1.2.3-1.src"`
		`+ }`
		`+ ]`
		`+ }]`
		`+ },`
		`+ ])`
		`+`
		`+ @patch('freshmaker.lightblue.LightBlue.find_container_repositories')`
		`+ @patch('freshmaker.lightblue.LightBlue.find_container_images')`
		`+ @patch('freshmaker.kojiservice.KojiService.get_build')`
		`+ @patch('freshmaker.kojiservice.KojiService.get_task_request')`
		`+ @patch('os.path.exists')`
		`def test_images_with_content_set_packages_beta(`
		`self, exists, koji_task_request, koji_get_build, cont_images, cont_repos):`
		`exists.return_value = True`

cqi commented 4 years ago

Signed-off-by: Chenxiong Qi cqi@redhat.com

rebased onto 4170f05a1e90e9e88c59848c5955938442bb6ee4

4 years ago

rebased onto e3e4dc7a5e726825443611021752e2d06f4f8f4d

4 years ago

rebased onto 0722d46254f602bc275215f652c47edef3d3463e

4 years ago

rebased onto 00cfba2ff11cfdbba161b92e2d05b059dd143376

4 years ago

qwan commented on line 42 of freshmaker/lightblue.py 4 years ago

I think the for loop is clearer and easier to read, though it can be improved to use set instead of list, so we can remove seen_name_versions. what do you think?

qwan commented on line 18 of freshmaker/lightblue.py 4 years ago

If we don't need the the nvr_key_func in the change I previously commented, I'd say it's unnecessary to replace the lambda with a nested function.

cqi commented on line 42 of freshmaker/lightblue.py 4 years ago

I think the for loop is clearer and easier to read, though it can be improved to use set instead of list, so we can remove seen_name_versions. what do you think?

What the for-loop does is to group images by name-version and just get the top (latest) image from a name-version group. The grouping action is just what groupby does and using groupby can express the intention much straightforward. When someone reads the code, he/she does not need to translate the meaning of whole for-loop in mind. So, why not use a higher abstraction to make the code easier to read and understand?

gnaponie commented 4 years ago

I personally like the lambda approach more, it's just easier to understand in my opinion... but that's just my opinion.
Any other opinions?

lucarval commented on line 18 of freshmaker/lightblue.py 4 years ago

Alternatively, you could just add a property to Containerimage to return the nvr?

@property
def nvr(self):
  return self['brew']['build']

Then you could even make sorted_by_nvr smart enough to detect this attr, and use it if available.

elif hasattr(item1, 'nvr') and hasattr(item2, 'nvr'):
  nvr1 = item1.nvr
  nvr2 = item2.nvr

rebased onto 4735133b6e43580edfecbecda2ecf895ccda7d11

4 years ago

rebased onto 97b204fef69acedd9670ae3085987408af5c34d4

4 years ago

cqi commented 4 years ago

@lucarval Thanks for the idea. PR is updated accordingly.

lucarval commented on line 81 of freshmaker/lightblue.py 4 years ago

item.nvr ? :)

lucarval commented on line 80 of freshmaker/lightblue.py 4 years ago

This function doesn't actually group items. It returns the name and version for a given item. Can we rename it accordingly? e.g. _get_name_and_version ?

rebased onto d224861

4 years ago

cqi commented 4 years ago

Thanks @lucarval PTAL.

BTW, there are still many image["brew"]["build"] in the code. I'll replace them with image.nvr after this PR is merged.

gnaponie commented 4 years ago

Looks good :) +1

lucarval commented 4 years ago

:thumbsup: Thanks for refactoring this piece of the code!

cqi commented 4 years ago

Commit 336eb7a fixes this pull-request

Pull-Request has been merged by cqi