Screwdriver: Proposal for notification plugin interface changes

[catto]

Issue

I found that notification plugins are registered at registerPlugins.js#L85 and called notify, then it returns a Promise but registerPlugins doesn't uses the status of the Promise.

Also, notify listens on a certain event inside a callback of the Promise. This event will be fired many time, so resolve and reject in the callback of the event will be invoked many times. It doesn't make much sense.

Proposal

From the above, I propose changes for notification plugin interface.

• Create register function in notifications-base. It registers notify function for specified event.
  
  
  
  • server.on(this.eventName, notify)
  
  
• notifications-base have notify and _notify function.
  
  
  
  • The same as current ones
  
  
• screwdriver/lib/registeryPlugins "require"s notification plugins and calls register of each plugin instead of calling notify.
• Each notification plugin has _notify function which processes every notification event.
  
  
  
  • Mostly the same as a current event callback
  
  

How about these changes?

[FenrirUnbound]

I guess I missed the initial spec for notifications so I may be missing some context behind it. Looking at how notifications-email is currently structured, it's trying really hard to be a HapiJS plugin, but doesn't do it.

I really like your suggestion, since the interface is less HapiJS-specific and focuses what a notification plugin should be allowed to do. As you pointed out, we only need a couple of things from the notification plugin:

• The events it cares about
• A notify() hook.

Instead of a register function, we could instead change the notification interface to provide both of these pieces of information:

• eventNames - property that returns an array of events that the notification wants to respond to
• notify - which already is there.

Then the logic of registering plugins just turns into something along the lines of:

Object.keys(notificationConfig).forEach((plugin) => {
  const Plugin = require(`screwdriver-notifications-${plugin}`);
  const notificationPlugin = new Plugin(
    notificationConfig[plugin], server,  'build_status');

  notificationPlugin.events.forEach((eventName) => {
    server.on(eventName, notificationPlugin.notify);
  });
}

This way the plugin doesn't touch the server object at all, and the framework restricts it so that the plugin can only act on a certain event.

[catto]

I like your idea. I think it would be better to set events as 'build_status' in notifications-base module so that plugin developers can override or add events in it.

If you are okay for this, I will make changes and submit PRs to repositories.

[FenrirUnbound]

Sounds good to me! Give it a try and send a PR 🚀 . It'll give us an opportunity to see it in code instead of as an abstract concept

[d2lam]

Plugged this in and confirmed that it is working 👍 Thanks @tk3fftk @catto