Add new webhooks plugin that superseed github and rollbar plugins.

[francois2metz]

Information

This is a WIP of a new plugin webhooks that allow multiple sub-webhooks as discused on #1287 and #1248

The config looks like

[[inputs.webhooks]]
  service_address = ":1925"

  [[inputs.webhooks.webhook]]
    name = "rollbar"
    path = "/rollbar"

  [[inputs.webhooks.webhook]]
    name = "github"
    path = "/github"

There is still work to do. We are waiting for feedbacks before :)

• update config

Required for all PRs:

• CHANGELOG.md updated
• Sign CLA (if not already signed)
• README.md updated (if adding a new plugin)

Signed-off-by: François de Metz [email protected]
Signed-off-by: Cyril Duez [email protected]

[francois2metz]

Is this approach ok for you? cc @sparrc

[sparrc]

This could be problematic as future webhooks could likely have very different necessary configuration parameters.

I would prefer if we could directly define different types of webhooks here. This would introduce the limitation that only one type of webhook would be allowed per-port, but I think this is OK.

ie, config would look like this:

[[inputs.webhook]]
  service_address = ":1619"
  [inputs.webhooks.github]
    path = "/github"
  [inputs.webhooks.rollbar]
    path = "/rollbar"

[cduez]

We change the suggested configuration because we couldn't find a nice and effective way to easily detect configured webhooks plugins with it.

Correct me if i am wrong, but with this configuration i would have a struct like this :

type WebhookConfig struct {
    ServiceAddress string

    RollbarConfig RollbarConfig
    GithubConfig  GithubConfig
}

type RollbarConfig struct {
     path string
}

type GithubConfig struct {
     path string
}

Even with this configuration:

[[inputs.webhook]]
  service_address = ":1619"
  [inputs.webhooks.github]
    path = "/github"

The problems are:

1. The RollbarConfig get initialized so we don't know if we should start it or not.
2. Furthermore, this probably mean that we can't iterate over a list of webhooks plugins and we have to manually iterate like this:

plugins := [wb.GithubConfig, wb.RollbarConfig]
for plugin := range plugins {
    plugin.Register...
}

Do you have any ideas of how we could solve this?

[sparrc]

Number 2 is true, you would need to maintain a list of the available webhook plugins.

Not sure I understand what you mean by Number 1, you should just be able to check if Github is not nil, and start it if so.

[cduez]

For the Number 1, we observed that all fields of the plugin struct are initialized.

This means that even if the rollbar configuration is missing, like the configuration above, we still get an initialized struct RollbarConfig (in WebhookConfig) with an empty field path. So the nil check doesn't work to know if the plugin is configured or not.

[cduez]

Oh nevermind, we just find that is how Go works. We just have to use a pointer instead it seems to work then.

[francois2metz]

Updated with the new config:

[[inputs.webhook]]
  service_address = ":1619"
  [inputs.webhooks.github]
    path = "/github"

This PR is ready for a full review.

[sparrc]

there's a risk of panic here on line 77, you should add in:

if !f.CanInterface() {
  continue
}

before calling Interface()

[francois2metz]

Ok. Fixed.

[sparrc]

Cursory review and I think it looks great, I am swamped with some other work right now but will try to do a full review of this over the weekend.

thanks @cduez & @francois2metz, is this done & ready from your perspective?

[sparrc]

in here I believe you should also check if wbPlugin == nil ? (and don't append if it is nil)

[sparrc]

getting hairy but I think you actually need reflect.ValueOf(wbPlugin).IsNil()

[francois2metz]

Fixed, with some tests.

[francois2metz]

It's done on our side. I'll check your comments.

[francois2metz]

Updated with your comments.

[francois2metz]

All done @sparrc