artifact_type is repeated by passing strings, e.g. 'module', 'image', the same happens in method record_build. Suggest to give these values a name, so that types can be referenced, e.g. ArtifactType.Image or ArtifactType.Module.

I don't see any problem here, however I have another thought to make this piece of code simpler, just FYI.

Calling allow_build aims to filter out what containers should be rebuilt. From this point of view, that call can happen outside of for loop. For example,

...
containers = self.get_containers_including_rpms(rpms)
containers = [container for container in container
              if allow_build(event, 'image', container['name'], container['branch']]
for container in containers:
    ...

Why not also change the class name as well?

I think it is cosmetic issue, but I like to handle situations like this one following way:

if not self.allow_build(event, 'image', event.repo, event.branch):
    log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist",
        event.repo, event.branch)
    return
try:
    task_id = self.build_image(repo_url=event.repo_url,
    ....

It makes the code easier to read to me, keeps the indentation lower and makes it easier to add another condition on which the module should not be rebuild.

What do you think?

I added names for these two class to prevent errors (because allow_build in this commit need to inspect the hander's name), and don't want to change the existing class name in this commit as I have an proposal on re-organizing the handlers in another way of 'each handler will handle the events from a source, like dist-git, buildsys, mbs', and then can rename some of the classes to follow a consistent style, will discuss this with team later.

+1

The same cosmetic issue I have mentioned above for Docker images.

Agree on this, will update.

And here too, something like:

if not self.allow_build(...):
    continue

@cqi: Hm, when thinking about it, we won't get the log.info() here with your code. Not sure how important it is...

@jkaluza, you're right, that's why I didn't filter the modules/images first, skipping the build targets silently is not good, in this way we will need two loops, one for printing the skipping info, and list operation to get modules/images which are/not filtered out.

I think changing the code to following code-flow is the best approach in this situation. I think we should log.info in this case, you are right here.

if not self.allow_build(....):
    log.info(...)
    continue
....

I lost my comment after rebase.

Defining a separate method as filter method would be helpful to do more against one "container" besides calling allow_build, e.g. logging something.

To avoid iterating containers twice, just need to convert get_containers_including_rpms to a generator instead of returning a list object.

I think we can do that in another PR. I've created bug to track this: https://pagure.io/freshmaker/issue/30

Sorry, I'm not very clear with the solution of returning generator here, could you explain more?

Sorry, generator in this case cannot help to reduce a loop.

Seems, like all comments are addressed, so +1.