Improvements and notifications on all hooks

[LevelbossMike]

No description provided.

[LevelbossMike]

Because of the guard on line 37 we can just return a falsy value for url, headers, method or body and the plugin won't notify the service. I'm not too stoked about the api here. So maybe we could mark configuration options as 'required' to make this more elegant.

[marcoow]

I guess this should just result in an error saying the option is required?

[LevelbossMike]

This would make it difficult to provide preconfigured services that notify on one hook per default (I.e. bugsnag). With this falsy value approach we just return a falsy value and the service won't be notified. If we errored out all users would have to opt-out of bugsnag which seems kind of annoying to me

[marcoow]

Ah, I see. But shouldn't users have to opt-in into services anyway? Wouldn't the current implementation mean that the Bugsnag service would always be used as soon as the addon is installed? I'd say users should have to explicitly opt-in by at least configuring

bugsnag: {
  apiKey: 'some',
  didActivate: true
}

and with only

bugsnag: {
  apiKey: 'some'
}

it would just not do anything. Maybe we should have a warning for that case then saying that bugsnag was configured but not actually activated for any hook. I think that way it would be more explicit.

[LevelbossMike]

That's the way it's working right now without the warning. Bugsnag is kind of special as it preconfigures to notify on didActivate. We can remove that of course and don't default to didActivate.

[marcoow]

But then the way to activate Bugsnag would be to define the API key which think is a bit implicit. I think it's nicer to separate configuration and activation and use the same behavior for all services.

[LevelbossMike]

The required option thing for services could also be implemented as a Promise rejection when we move to a promise based configuration of services.

services.slack = {
  url: function(context) {
    var webhookURL = this.webhookURL;

    if (!webhookURL) { return Promise.reject(); }

    return webhookURL;
  }
  // ...
}

For the time being I guess this falsy approach is enough and we should stick with it until a need for a promise based configuration of services arises.

[oliverbarnes]

lgtm 👍

[marcoow]

This behavior has been remove actually.

[LevelbossMike]

For people that follow this discussion: This hasn't been removed actually but is an implementation detail that you should not rely on when creating preconfigured services (i.e. it is discouraged to create preconfigured services that default to notify on any hook. This should instead be done by users directly in config/deploy.js)