freshmaker

Clone

#522 Bug fix: affected packages not correctly detected
Merged 4 years ago by gnaponie. Opened 4 years ago by gnaponie.
gnaponie/freshmaker correct-affected-modules  into  master

file modified
+14 -2 
@@ -36,6 +36,7 @@ 
 

  from freshmaker.types import (
 

      ArtifactType, ArtifactBuildState, EventState, RebuildReason)
 

  from freshmaker.models import Event, Compose
 

+ from freshmaker.utils import is_pkg_modular
 

  

  

  class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler):
 
@@ -424,8 +425,19 @@ 
 

          # change was made, and rebuild everything.
 

          affected_pkgs = set([pkg['pkg_name'] for pkg in self.event.advisory.affected_pkgs])
 

          if affected_pkgs:
 

-             tmp_srpm_nvrs = srpm_nvrs
 

-             srpm_nvrs = set([srpm_nvr for srpm_nvr in srpm_nvrs if kobo.rpmlib.parse_nvr(srpm_nvr)["name"] in affected_pkgs])
 

+             tmp_srpm_nvrs = set(srpm_nvrs)
 

+             srpm_nvrs = set()
 

+             for srpm_nvr in tmp_srpm_nvrs:
 

+                 srpm_name = kobo.rpmlib.parse_nvr(srpm_nvr)["name"]
 

+                 # In case the SRPM NVR is modular, the `affected_pkgs` might contain
 

+                 # modules which are in the "module_name:module_stream/pkg_name" format.
 

+                 # We need to respect this format and only try to match the package name, it
 

+                 # means only the "/pkg_name" part of affected_pkg.
 

+                 if is_pkg_modular(srpm_nvr):
 

+                     if any(affected_pkg.endswith(f"/{srpm_name}") for affected_pkg in affected_pkgs):
 

+                         srpm_nvrs.add(srpm_nvr)
 

+                 elif srpm_name in affected_pkgs:
 

+                     srpm_nvrs.add(srpm_nvr)
 

              self.log_info(("Not going to rebuild container images with RPMS from these SRPMs "
 

                             "because they're not affected: %r"), tmp_srpm_nvrs.difference(srpm_nvrs))

file modified
+3 -3 
@@ -36,7 +36,7 @@ 
 

  

  from freshmaker import log, conf
 

  from freshmaker.kojiservice import koji_service
 

- from freshmaker.utils import sorted_by_nvr
 

+ from freshmaker.utils import sorted_by_nvr, is_pkg_modular
 

  import koji
 

  

  
@@ -749,8 +749,8 @@ 
 

              # name in the image is also modular. Also, include the image if the opposite is true.
 

              for rpm in rpms or []:
 

                  for srpm_nvr in srpm_name_to_nvrs.get(rpm.get("srpm_name"), []):
 

-                     if (("module+" in srpm_nvr and "module+" in rpm["srpm_nevra"]) or
 

-                             ("module+" not in srpm_nvr and "module+" not in rpm["srpm_nevra"])):
 

+                     if ((is_pkg_modular(srpm_nvr) and is_pkg_modular(rpm["srpm_nevra"])) or
 

+                             (not is_pkg_modular(srpm_nvr) and not is_pkg_modular(rpm["srpm_nevra"]))):
 

                          ret.append(image)
 

                          image_included = True
 

                          break

file modified
+5 
@@ -184,3 +184,8 @@ 
 

          raise OSError("Got an error (%d) from %s: %s" % (p1.returncode, command[0], err))
 

  

      return out
 

+ 

+ 

+ def is_pkg_modular(nvr):
 

+     """ Returns True if the package is modular, False otherwise. """
 

+     return "module+" in nvr

file modified
+17 
@@ -553,6 +553,23 @@ 
 

              published=True, release_categories=conf.lightblue_release_categories,
 

              leaf_container_images=None)
 

  

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={
 

+         'RebuildImagesOnRPMAdvisoryChange': {
 

+             'image': {'advisory_name': 'RHBA-*'}
 

+         }
 

+     })
 

+     @patch('os.path.exists', return_value=True)
 

+     def test_affected_packages_with_modules(self, exists):
 

+         self.event.advisory.affected_pkgs = [{'product': 'rhel-8', 'pkg_name': 'nodejs:10/nodejs'}]
 

+         self.get_builds.return_value = ["nodejs-10.19.0-1.module+el8.1.0+5726+6ed65f8c.x86_64"]
 

+         self.handler._find_images_to_rebuild(123456)

Do you want to verify the output of this?

 
+ 

+         self.find_images_to_rebuild.assert_called_once_with(
 

+             set(['nodejs-10.19.0-1.module+el8.1.0+5726+6ed65f8c.x86_64']), ['content-set-1'],
 

+             filter_fnc=self.handler._filter_out_not_allowed_builds,
 

+             published=True, release_categories=conf.lightblue_release_categories,
 

+             leaf_container_images=None)
 

+ 

  

  class TestAllowBuild(helpers.ModelsTestCase):
 

      """Test RebuildImagesOnRPMAdvisoryChange.allow_build"""

When the affected packages are modular RPMs, sfm2 uses a different
format: module_name:module_stream:affected_package_name while normally
it is simply affected_package_name. With this patch we enhance
Freshmaker to compare differently this when it is for a modular RPM.

ref: CLOUDWF-1038

Signed-off-by: Giulia Naponiello gnaponie@redhat.com

rebased onto d6503f4cd4417a587f970e99a15450e5c7ddbfa4
4 years ago

Could you break out of the for affected_pkg in affected_pkgs: loop after this line?

Could you break out of the for affected_pkg in affected_pkgs: loop after this line?

Are you sure? Shouldn't we find all of them and keep looking if we find one?

Optional:
It'd likely be more efficient if you broke affected_pkgs to be affected_pkgs and affected_modular_pkgs. Then you don't have to loop through all the affected packages every time a modular SRPM is encountered.

You could populate it like this:

affected_pkgs = set()
affect_modular_pkgs = set()
modular_regex = r'(?:.+\:.+)(?:\/)(?P<package>.+)'
for pkg in self.event.advisory.affected_pkgs:
    pkg_name = pkg['pkg_name']
    modular_package = getattr(re.match(modular_regex, pkg_name), 'groupdict', dict)().get('package')
    if modular_package:
        affect_modular_pkgs.add(pkg_name)
    else:
        affected_pkgs.add(pkg_name)
Edited 4 years ago by mprahl

Could you break out of the for affected_pkg in affected_pkgs: loop after this line?

Are you sure? Shouldn't we find all of them and keep looking if we find one?

The only thing that happens in this loop is srpm_nvrs.add(srpm_nvr). Even if the condition is True a second time, srpm_nvrs would be the same.

Why is this loop necessary. Can't you just call self.handler._find_images_to_rebuild(123456) directly?

Could you break out of the for affected_pkg in affected_pkgs: loop after this line?
Are you sure? Shouldn't we find all of them and keep looking if we find one?

The only thing that happens in this loop is srpm_nvrs.add(srpm_nvr). Even if the condition is True a second time, srpm_nvrs would be the same.

You are right, because what we are adding remains srpm_nvr. Maybe we could make this even more efficient (just one line, using maybe any), I can check that.

Could you break out of the for affected_pkg in affected_pkgs: loop after this line?
Are you sure? Shouldn't we find all of them and keep looking if we find one?
The only thing that happens in this loop is srpm_nvrs.add(srpm_nvr). Even if the condition is True a second time, srpm_nvrs would be the same.

You are right, because what we are adding remains srpm_nvr. Maybe we could make this even more efficient (just one line, using maybe any), I can check that.

Yes, using any would be nice.

rebased onto 16588f9c94fa90554613786f17f967db9d1f7f95
4 years ago

rebased onto d2373ba005c3899f9faa0bca418f36e8ca8a194a
4 years ago

@mprahl I changed it with any, but I'm not really sure this is actually more efficient :D It is for sure more elegant. Anyhow... could you review once again? Thank you.

This logic seems to be duplicated in several places. Could you please move this check to a separate function called something like is_package_modular?

Optional:
It'd be nice to use an f string here:

if any(affected_pkg.endswith(f"/{srpm_name}") for affected_pkg in affected_pkgs):

Do you want to verify the output of this?

Do you want to verify the output of this?

The test as it is already catches the bug that we are fixing with the PR (I tested it removing the changes, and it correctly fails). The data in the test are fake, I should make the test more complex in order to have images that actually make sense to be able to check them.
Also the other tests in here are not checking the output. So I would accept this test as it is.

rebased onto e4938c2
4 years ago

@mprahl comments addressed. Could you review once again? Thank you.

:thumbsup:

rebased onto 69e2b60
4 years ago

A rebased once again simply to remove conflicts. I'll go ahead and merge this.
Thank you Matt for the review!

Pull-Request has been merged by gnaponie
4 years ago
Changes Summary 4
+14 -2
file changed
freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py
+3 -3
file changed
freshmaker/lightblue.py
+5 -0
file changed
freshmaker/utils.py
+17 -0
file changed
tests/handlers/koji/test_rebuild_images_on_rpm_advisory_change.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.