Skip to end of metadata
Go to start of metadata

Actual implementation

After a general look at notification process, I think it isn't well implemented. I see that some specific code for each notifier are hard coded in several classes like :

  • getRecipients in ContinuumRecipients
  • getNagEmailAddress in DefaultMavenBuilderHelper
  • ...

It isn't simple to add a new notifier in continuum, so we need to refactor the notification mechanism.
I don't understand why we have <address> under <notifier> in pom. It isn't use by all notifier. I think we must use it in <configuration> instead when it's necessary.

Icon

I think we should have a the same structure in ContinuumProject as we do in MavenProject for the notifiers. So a list of notifiers, where each notifier specifies a type and a configuration. I believe you are correct in that the address field isn't required. So in the m2 project builder we simply copy over the notifiers from the MavenProject to the MavenTwoProject (extends ContinuumProject). For the m1 project builder we can create a Notifier list on the fly using the nag email address in the m1 project. We can also do the same with the ant/shell projects as an email address is specified for now. We will want to make this more flexible so that the user can select from the available notification mechanisms but cleaning this up and allowing for it in the near future I think will suffice in this round of refactoring. (jvz)

Proposal:

  • Remove <adress> under <notifier> in pom
    Icon

    +1 (jvz), +1 (trygve)

  • Refactor plexus-notifier for remove recipient/recipientSource. if a notifier need some recipients like mail and jabber, it find them in the context
    Icon

    The Notifier should probably be threadsafe so it might be best to change the signature to include a configuration (Properties) that can be used to setup the notifier. So maybe Notifier.sendNotification( String messageId, Set recipients, Map configuration, Map context ). I think having a list of recipients is still a good idea to be clear. I think we could probably also get rid of the NotificationDispatcher as we probably don't want to cycle through all the available notifiers because they may not apply to a particular case. For example we may have 10 notifiers but a given ContinuumProject only uses 1. We should probably just let the client use the NotifierManager directly and pick out what Notifier it wants to use. So I'd probably get rid of everything in the org.codehaus.plexus.notification package and bring the notifiers up into that package. (jvz)

    Another thought is that we may just want to provide the message text directly to the notifier. The messageId is good in the case of velocity where you want to lookup a template but you may just want to punt in a String. So maybe the addition of Notifier.sendNotification( String message, Set recipients, Properties configuration ); I think the messageId goes hand in hand with using a context and might be velocity/template specific.

    Icon

    Making the notifiers more configurable is a good thing, I imagine people would like to configure SMTP and Jabber servers that their notifiers would use.

    I'm -1 to removing the recipient source, this will tie the notifiers to Continuum and I don't see what we gain. (Ok, so the notifiers are already continuum specific but they don't have to)

    Jason: why do you want to remove the notification dispatcher and have spread out the notification out all over the core? Why should each part of the core decide which notifiers (mail, jabber, etc) that should get notified? Won't this make it much harder to add another notifier as you'll have to find all the places in the core where you're doing notification.

    Not that not all notifiers will be executed on each even, the code in Plexus Notification will only call the notifiers if there are more than one recipient for a event. (trygve)

  • Remove all notifier specific code from continuum classes
    Icon

    +1 (jvz)

    Icon

    What core are you referring to? (trygve)

  • Load notifiers from pom declaration and configure them with notifier configuration
    Icon

    I think here we would lookup the appropriate notifiers using the NotifierManager and then pass in the configuration as a parameter. There is no way to lookup a component with a configuration currently (jvz)

    Icon

    Making the ContinuumProject reflect the MavenProject is a good idea, no doubt. But once we've created the ContinuumProject we won't have any hard references to the MavenProject so the notifiers won't be able to read directly from the POM anymore. But if we're modelling the ContinuumProject as the MavenProject my point is moot. (trygve)

  • Create a documentation generator tool for generate notifier configuration guide with all possible properties.
Icon

Desirable but this can come a bit later

  • No labels