Webhook notify #1095

bakurin · 2021-08-09T18:43:16Z

This is a PR for the requested feature [#924]

There are not many details in the request, so feel free to specify any requirements. Any feedback is appreciated.

paskal

A bunch of minor things here and there, generally looks good. Thank you!

backend/app/cmd/server.go

backend/app/notify/webhook.go

paskal

Same comments from previous time, plus I reviewed the test coverage.

backend/app/notify/webhook.go

backend/app/notify/webhook_test.go

backend/app/notify/webhook.go

paskal

Just one more thing, please write about this new notification method for admins in the Readme and present its command-line options in the table there.

umputun

thx. just a few questions & suggestions

umputun · 2021-08-11T20:28:23Z

backend/app/cmd/server.go

@@ -230,6 +230,12 @@ type NotifyGroup struct {
 		Token   string `long:"token" env:"TOKEN" description:"slack token"`
 		Channel string `long:"chan" env:"CHAN" description:"slack channel"`
 	} `group:"slack" namespace:"slack" env-namespace:"SLACK"`
+	Webhook struct {
+		WebhookURL string        `long:"url" env:"URL" description:"webhook notification URL"`
+		Headers    string        `long:"headers" env:"HEADERS" description:"webhook authentication headers in format --notify.webhook.headers=Header1:Value1,Header2:Value2,..."`


I'm not sure if combining headers into a string and splitting it after is such a good idea. Is there a reason not to have Headers as []string ?

btw, this is how i did it in in reproxy https://github.com/umputun/reproxy/blob/master/app/main.go#L36

I like []string idea. I brought splitAtCommas from reproxy project. Added it to server.go. Is that OK?

umputun · 2021-08-11T20:31:18Z

backend/app/notify/webhook.go

+// where key is an HTTP header name, and value is a list of header values
+// For example parameter string Header1:Value1,Header1:Value2,Header2:Value3
+// will produce a map {Header1: [Value1, Value2], Header2: [Value3]}
+func buildHeaders(opt string) webhookHeaders {


I don't think it will survive a complex headers, like this one:

Access-Control-Allow-Headers:"DNT,X-CustomHeader,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type"

see the similar issue here umputun/reproxy#100 and the solution proposed and implemented

Let's go with splitAtCommas instead

umputun · 2021-08-11T20:33:05Z

backend/app/notify/webhook.go

+)
+
+const (
+	webhookTimeOut  = 5 * time.Second


nit: probably calling it "default" will clarify the intent better, i.e. webhookDefaultTimeOut

umputun · 2021-08-11T20:34:06Z

backend/app/notify/webhook.go

+func NewWebhook(params WebhookParams) (*Webhook, error) {
+	res := &Webhook{WebhookParams: params}
+	if res.WebhookURL == "" {
+		return nil, errors.New("webhook webhook URL is required for webhook notifications")


"webhook webhook" looks like dbl by mistake

umputun · 2021-08-11T20:40:51Z

backend/app/notify/webhook.go

+		}
+	}
+
+	client := http.Client{Timeout: t.Timeout}


not a big fan of making http client directly. Passing it as dependency will allow to redefine the client with custom transport or even use https://github.com/go-pkgz/requester to provide advanced things like repeater, limiter, circuit breaker and so on. As long as Webhook accept injection of smth with Do(req) interface the client can be enhanced from the outside

Ok. Makes sense.

umputun · 2021-08-11T20:42:46Z

backend/app/notify/webhook.go

+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		respBody, _ := ioutil.ReadAll(resp.Body)


any good reason why the error ignored here? if connection failed in the middle this call may fail

The request body is no big deal here. It is used only for debugging to make the error messages more informative. How would you suggest handling it?

if this is dbg only, probably smth like this?

if resp.StatusCode != http.StatusOK { respBody, err := ioutil.ReadAll(resp.Body) if err != nil { log.Printf("[DEBUG] can't blah blah, %v", err) .... } .... }

umputun

sorry, missed it.
Just a few minor things

umputun · 2021-08-26T05:37:35Z

backend/app/cmd/server.go

+		client := &http.Client{Timeout: 5 * time.Second}
+		webhookHeaders := s.Notify.Webhook.Headers
+		if len(webhookHeaders) == 0 {
+			webhookHeaders = splitAtCommas(os.Getenv("HEADER")) // env value may have comma inside "", parsed separately


this env name seems to be borrowed from the reproxy as-is. In remark42 this likely will be NOTIFY_WEBHOOK_HEADERS to make it similar to all the automatic evn parameters used everywhere. Another thing - this was easy to miss because there is no integration tests for something like TestMain_WithWebhook. I think making such a test should be doable and will be very nice to have.

You are right, I copy-pasted the code snippet and forgot to change the var name. Regarding the integration test, I didn't find any example in the project, so please correct me if my implementation looks weird.

all tests in cmd/server_test.go are integration tests

Yes. I saw them. But idea was to test webhook notifier initialization based on ENV variables, wasn't it? Do you think it doesn't worth it to have such tests in the main package?

sure, this test is helpful. Usually, we don't test passing params via env as this is handled by flags library but this case is different due to the custom parsing

umputun · 2021-08-26T05:39:44Z

backend/app/notify/webhook.go

+	webhookDefaultTemplate = `{"text": "{{.Text}}"}`
+)
+
+type webhookClient interface {


it is kind of unusual to define a private interface for the injection. I would suggest making this public as it will communicate intent better

umputun · 2021-08-26T05:42:10Z

backend/app/notify/webhook.go

+	res.webhookClient = client
+	res.webhookTemplate = payloadTmpl
+
+	log.Printf("[DEBUG] create new Webhook notifier for %s", res.WebhookURL)


nit: inconsistent, as in other places it is webhook (lower-case w)

umputun · 2021-08-26T05:51:23Z

backend/app/notify/webhook.go

+	return nil
+}
+
+func (t *Webhook) doSend(ctx context.Context, payload io.Reader) error {


I'm not sure if doSend should be even a function and not part of the Send. Is there a good reason to isolate the code? it is not doing just send anyway as it prepares request too. I'd rather put all of this to Send but this is just a minor nit, up to you

I did merge it.

umputun · 2021-08-28T03:56:37Z

@bakurin pls rebase/squash all the commits from this PR into a single one as you are ready to merge it

bakurin · 2021-08-28T16:31:17Z

The branch has been squashed/rebased and is ready to merge.

umputun

LGTM. The missing part is some documentation describing the new functionality, but it can be added separately

thx

bakurin requested a review from umputun as a code owner August 9, 2021 18:43

umputun requested a review from paskal August 9, 2021 18:44

paskal requested changes Aug 9, 2021

View reviewed changes

bakurin requested a review from paskal August 10, 2021 19:58

paskal requested changes Aug 11, 2021

View reviewed changes

paskal previously approved these changes Aug 11, 2021

View reviewed changes

bakurin dismissed paskal’s stale review via 3d090ce August 11, 2021 19:57

paskal previously approved these changes Aug 11, 2021

View reviewed changes

umputun requested changes Aug 11, 2021

View reviewed changes

bakurin dismissed paskal’s stale review via 5e98f4c August 12, 2021 19:17

bakurin requested a review from umputun August 16, 2021 18:35

umputun requested changes Aug 26, 2021

View reviewed changes

umputun reviewed Aug 26, 2021

View reviewed changes

Webhook destination for notifications

0896bf4

bakurin force-pushed the webhook-notify branch from f5ff2f1 to 0896bf4 Compare August 28, 2021 16:18

umputun approved these changes Aug 28, 2021

View reviewed changes

umputun requested a review from paskal August 28, 2021 16:37

paskal approved these changes Aug 28, 2021

View reviewed changes

umputun merged commit be46e84 into umputun:master Aug 28, 2021

paskal mentioned this pull request Dec 19, 2021

Feature: Webhook Notify #924

Closed

paskal added this to the v1.9 milestone Jan 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook notify #1095

Webhook notify #1095

bakurin commented Aug 9, 2021

paskal left a comment

paskal left a comment

paskal left a comment

umputun left a comment

umputun Aug 11, 2021 •

edited

Loading

bakurin Aug 12, 2021

umputun Aug 11, 2021 •

edited

Loading

bakurin Aug 12, 2021

umputun Aug 11, 2021 •

edited

Loading

bakurin Aug 12, 2021

umputun Aug 11, 2021

bakurin Aug 12, 2021

umputun Aug 11, 2021

bakurin Aug 12, 2021

umputun Aug 11, 2021

bakurin Aug 12, 2021

umputun Aug 12, 2021

umputun left a comment

umputun Aug 26, 2021

bakurin Aug 26, 2021

umputun Aug 26, 2021

bakurin Aug 26, 2021

umputun Aug 26, 2021

bakurin Aug 27, 2021

umputun Aug 26, 2021

bakurin Aug 26, 2021

umputun Aug 26, 2021

bakurin Aug 26, 2021

umputun Aug 26, 2021 •

edited

Loading

bakurin Aug 26, 2021

umputun commented Aug 28, 2021

bakurin commented Aug 28, 2021

umputun left a comment

Webhook notify #1095

Webhook notify #1095

Conversation

bakurin commented Aug 9, 2021

paskal left a comment

Choose a reason for hiding this comment

paskal left a comment

Choose a reason for hiding this comment

paskal left a comment

Choose a reason for hiding this comment

umputun left a comment

Choose a reason for hiding this comment

umputun Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umputun Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umputun Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umputun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umputun Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umputun commented Aug 28, 2021

bakurin commented Aug 28, 2021

umputun left a comment

Choose a reason for hiding this comment

umputun Aug 11, 2021 •

edited

Loading

umputun Aug 11, 2021 •

edited

Loading

umputun Aug 11, 2021 •

edited

Loading

umputun Aug 26, 2021 •

edited

Loading