This doesn't work under python3, giving the long module name here is not so convenient, I'm looking for a solution on this.

Push this for review first, there is an issue about loading handlers under python3 which I pointed out in comment, I'm looking for a solution on this.

Do we really need introduce another complicated level by FedMsgFactory and subclasses to generate fedmsgs?

Currently, it is easy for me to pick up a fedmsg from datagrepper, make small changes for running tests. For me, that is much straightforward than understanding those classes and calling API to make changes.

Instead of putting build_module and build_container into BaseHandler, I would suggest to separate module build and container build into individual classes, like this

https://drive.google.com/a/redhat.com/file/d/0Byp_laBFYpJmdVpXd0ZTN1F0MXM/view?usp=sharing

In this way, it would be easy for us to add more code for module build and container build individually and avoid putting all things into one big class BaseHandler.

Added a new commit to workaround the issue of loading handlers in python3.

Do we really need introduce another complicated level by FedMsgFactory and subclasses to generate fedmsgs?
Currently, it is easy for me to pick up a fedmsg from datagrepper, make small changes for running tests. For me, that is much straightforward than understanding those classes and calling API to make changes.

These test helpers are not introduced in this PR, so we can ignore this in this PR, we can have a talk on this later and do something to improve this if needed.

Instead of putting build_module and build_container into BaseHandler, I would suggest to separate module build and container build into individual classes, like this
https://drive.google.com/a/redhat.com/file/d/0Byp_laBFYpJmdVpXd0ZTN1F0MXM/view?usp=sharing
In this way, it would be easy for us to add more code for module build and container build individually and avoid putting all things into one big class BaseHandler.

Actually that's the original plan (put them in targets/mbs.py and targets/koji.py), but I found we can just keep build_module and build_container very simple in BaseHandler, since most of the stuff are done within freshmaker/mbs.py and freshmaker/kojiservice.py, so if we need to extend the functions, it should go into freshmaker/mbs.py or freshmaker/kojiservice.py.

freshmaker/mbs.py and freshmaker/kojiservice.py are different from the handlers. Both of them are responsible for interacting with external services to do the real job, whereas a handler handles the rebuild business logic.

When writing a new handler inheriting from BaseHandler for rebuilding docker image, which also has a method build_module. It's strange.

freshmaker/mbs.py and freshmaker/kojiservice.py are different from the handlers. Both of them are responsible for interacting with external services to do the real job, whereas a handler handles the rebuild business logic.

I don't see any difference, building modules/containers is also interacting with external services (mbs and koji).

When writing a new handler inheriting from BaseHandler for rebuilding docker image, which also has a method build_module. It's strange.

The meaning of a handler is handling a single type of event now, so a handler can do both building container and building module at the same time. An example is: when we have a container built from some modules, when the module metadata is changed in dist-git, we'll build both the module and containers using that module in a single handler, in this case , the handlers/git/module_metata_change.py:GitModuleMetadataChangeHandler.

Regarding the example, that is current freshmaker architecture can handle, which is similar to Publish/Subscribe in my mind.

That is, multiple handlers (module rebuild and docker image rebuild in the example) can handle (subscribe) an event (e.g. module metadata is changed in dist-git). This is implemented by can_handle in each handler and consume in FreshmakerConsumer together.

This separation could give a clear view and easy to understand I think.

If 9d45703 fixes fedmsg to work with Python 3 well, should this go to the upstream?

Regarding the example, that is current freshmaker architecture can handle, which is similar to Publish/Subscribe in my mind.

I'm not saying the current implementation will get problem when we need to deal with such cases. This PR just change the handler's meaning from doing a single type of stuff to *dealing with a single type of event` with an opposite point of view:

1. The current implementation: a handler is for building container images or building modules (btw, in this way, I'd prefer to change the current handler files to container.py and module.py), for the same handler it can be triggered by different events.

2. The proposed change in this PR: a handler is for dealing with a single type of event, for example, handlers/mbs/module_state_change.py:MBSModuleStateChangeHandler only responsible for dealing with module state change event from MBS, which means it can only be triggered by module state change event, but it can do both building containers and modules or other stuff. I personally think it's easy to find out what will happens when an event is triggered and easy to extend the use cases in this way.

So both the solutions has its advantage and disadvantage, such as with the currently solution, it's easy to find out what will trigger a container/module rebuild, with the proposal change, it's easy to find out what an event will trigger in freshmaker. I'd like to hear more comments for the proposal :)

If 9d45703 fixes fedmsg to work with Python 3 well, should this go to the upstream?

This is a special case which we need the workaround, usually the handlers/consumers are specified in package's entry-points, and python itself will load it just like what fedmsg.utils.load_class is doing, so I think we don't need to extend fedmsg to support this feature.

@cqi since you have concerns on this change, I uploaded another version of this change in #37 to request for comments. @jkaluza could you also have a review on both the 2 solutions and comment?

I think I like this PR more.

When I open the "handlers" directory and see more directories there, I kind of expect these directories are just another level how to divide handlers to groups. I then expect that "koji" lists all the handlers for "koji" related events and so on.

In the PR#37, the handlers are grouped by the "type of resulting artifact when they handled the event" - like "module" or "container". This is also good idea, but it is not so straightforward. When I open this directory for first time, I have a feeling that everything in "modules" directory are handlers for "module events", not handlers producing modules.

I also think that we will search for the handlers defined by source event (easy in this PR) more often than for all the handlers/events describing how module or container can be build (PR#37). I think so, because the Freshmaker is rebuilding artifacts per-source-event, and this event will be even stored in the database, so if we will search why particular module has been rebuild, we will probably query database to give us the event and then search for this event in the source code. It is easier to do that when you have events grouped by the source type and not by the resulting artifact type.

I'm open to other ideas, but we should ideally decide and merge one of the ways on Wednesday (given that cqi and qwan have state holiday on Monday/Tuesday).

Let's get this in. It'd better if separate container build and module build into individual classes, e.g.

                            +-------------+
             +--------------> BaseHandler <-------------+
             |              +-------------+             |
             |                                          |
             |                                          |
+------------+------------+                 +-----------+----------+
| ContainerRebuildHandler |                 | ModuleRebuildHandler |
+-------------------------+                 +----------------------+

Let's get this in. It'd better if separate container build and module build into individual classes, e.g.

Since the handler are grouped by source events in this PR, it's not so straightforward to have separate classes for building modules and containers, while it's easy in v2 #37. Personally I prefer this one since it has some minor advantages:

1. Have shorter handlers names than V2, because in V2, one event can both trigger module and container rebuild, so we need extra name in handler like 'GitRpmSpecChangeModuleHandlerandGitRpmSpecChangeContainerHandler` to distinguish container handler and module handler for the same event.

2. Some events may only responsible for updating db or something else that not build any artifact at all, so you can't easily put it under modules or containers in V2, you may need a common dir or something similar but not so meaningful, while in this version, it won't get any problem.

+1 on this, I see cqi still prefers v2 :( I hope he will still like Freshmaker even if we merge thisone :)