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)

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.

@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.

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

:thumbsup:

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