notification: store webhook and webhook delivery #413

alessandro-sorint · 2023-07-03T12:51:02Z

fix #417

alessandro-sorint · 2023-07-26T15:17:22Z

@sgotti can you do the review please?

internal/services/notification/runwebhookdeliveryevents.go

internal/services/notification/db/objects/objects.go

sgotti · 2023-07-27T07:30:13Z

internal/services/notification/runwebhookdeliverys.go

+		switch runWebhook.PayloadVersion {
+		case 1:
+			if err = json.Unmarshal([]byte(runWebhook.Payload), &webhook); err != nil {
+				return sequence, errors.WithStack(err)
+			}
+		default:
+			return sequence, errors.Errorf("unknown payload version: %d", runWebhook.PayloadVersion)


Why are you unmarshalling the payload?

internal/services/notification/runwebhooksheduler.go

internal/services/notification/runwebhookdeliverys.go

sgotti · 2023-07-27T07:37:06Z

internal/services/notification/runwebhookdeliverys.go

+
+	err := n.d.Do(ctx, func(tx *sql.Tx) error {
+		// start from last event
+		runWebhookDelivery, err := n.d.GetLastRunWebhookDelivery(tx)


Why are you starting from the last sequence? What happens if previous deliveries haven't been handled?

internal/services/notification/runwebhookdeliverys.go

sgotti · 2023-08-03T07:35:27Z

services/notification/types/runwebhookdelivery.go

+	NotDelivered  DeliveryStatus = "not_delivered"
+	Delivered     DeliveryStatus = "delivered"
+	DeliveryError DeliveryStatus = "delivery_error"


DeliveryStatusNotDelivered DeliveryStatus = "notDelivered" DeliveryStatusDelivered DeliveryStatus = "delivered" DeliveryStatusDeliveryError DeliveryStatus = "deliveryError"

sgotti · 2023-08-03T07:37:00Z

internal/services/notification/webhooksReceiver.go

@@ -0,0 +1,127 @@
+// Copyright 2023 Sorint.lab


no camel case in file names...

sgotti · 2023-08-03T07:38:29Z

internal/services/notification/notification_test.go

+
+	t.Logf("starting webhooks client")
+
+	time.Sleep(1 * time.Second)


This sleep is bad.

I added sleep because the test was failing. I thought the webhookreceiver was not started and needed much time.
I try to remove

webhookreceiver should start and return after is has been started.

internal/services/notification/db/objects/objects.go

sgotti · 2023-08-03T07:47:02Z

internal/services/notification/runwebhookdeliverys.go

+
+	if n.c.WebhookSecret != "" {
+		sig256 := hmac.New(sha256.New, []byte(n.c.WebhookSecret))
+		_, err = io.MultiWriter(sig256).Write(data)


why io.multiwriter?

sgotti · 2023-08-03T09:34:37Z

internal/services/notification/webhooks.go

-		sig256 := hmac.New(sha256.New, []byte(n.c.WebhookSecret))
-		_, err = io.MultiWriter(sig256).Write([]byte(body))
-		if err != nil {
+		if err := n.d.InsertOrUpdateRunWebhookDelivery(tx, runWebhookDelivery); err != nil {


Isn't this just an insert?

@sgotti I do not know what you mean

Here you're inserting a new entry, not updating an existing one. So use InsertBla and not InsertOrUpdateBla.

sgotti · 2023-08-03T09:34:45Z

internal/services/notification/notification_test.go

+		wh = types.NewRunWebhook(tx)
+		wh.Payload = "payloadtest"
+
+		if err := ns.d.InsertOrUpdateRunWebhook(tx, wh); err != nil {


Isn't this just an insert?

sgotti · 2023-08-03T09:34:52Z

internal/services/notification/notification_test.go

+		wd.DeliveryStatus = deliveryStatus
+		wd.RunWebhookID = runWebhookID
+
+		if err := ns.d.InsertOrUpdateRunWebhookDelivery(tx, wd); err != nil {


Isn't this just an insert?

sgotti · 2023-08-03T09:35:07Z

internal/services/notification/runwebhookdeliverys.go

+		runWebhookDelivery.Status = status
+		runWebhookDelivery.StatusCode = statusCode
+
+		if err = n.d.InsertOrUpdateRunWebhookDelivery(tx, runWebhookDelivery); err != nil {


Isn't this just an update?

sgotti · 2023-08-03T09:35:28Z

internal/services/notification/webhooks.go

-	if err != nil {
-		return errors.WithStack(err)
-	}
+		if err := n.d.InsertOrUpdateRunWebhook(tx, wh); err != nil {


Isn't this just an insert?

sgotti · 2023-08-03T09:37:09Z

internal/services/notification/runwebhookdeliverys.go

@@ -0,0 +1,198 @@
+// Copyright 2023 Sorint.lab


runwebhookdelivery.go

sgotti · 2023-08-03T09:38:04Z

internal/services/notification/runwebhookdeliverys.go

+)
+
+const (
+	MaxRunWebhookDeliverysLimit = 40


If you want to use the plural form it's deliveries

sgotti · 2023-08-03T09:39:20Z

internal/services/notification/db/db.go

+func (d *DB) GetRunWebhookDeliveryByID(tx *sql.Tx, runWebhookDeliveryID string) (*types.RunWebhookDelivery, error) {
+	q := runWebhookDeliverySelect()
+	q.Where(q.E("id", runWebhookDeliveryID))
+	runWebhookDeliverys, _, err := d.fetchRunWebhookDeliverys(tx, q)


the generated functions could have the wrong plural forms (since it only adds an s) but the manually typed code should use the right plural form (deliveries)

internal/services/notification/notification_test.go

sgotti · 2023-08-03T12:34:57Z

internal/services/notification/runwebhookdelivery.go

+
+	if n.c.WebhookSecret != "" {
+		sig256 := hmac.New(sha256.New, []byte(n.c.WebhookSecret))
+		_, err = io.WriteString(sig256, webhookPayload)


Use h256.Write ?

internal/services/notification/runwebhookdelivery.go

internal/services/notification/notification_test.go

internal/services/notification/runwebhookdelivery.go

sgotti · 2023-08-07T07:41:28Z

internal/services/notification/db/db.go

+
+func (d *DB) GetRunWebhookDeliveriesNotDeliveredAfterSequence(tx *sql.Tx, afterSequence uint64, limit int) ([]*types.RunWebhookDelivery, error) {
+	q := runWebhookDeliverySelect().OrderBy("sequence").Asc()
+	q.Where(q.E("delivery_status", types.DeliveryStatusNotDelivered))


This, in previous commits, was a query option to filter by delivery_status. Why was it removed and made fixed?

sgotti · 2023-08-07T07:43:41Z

internal/services/notification/db/db.go

+	return s[0], nil
+}
+
+func (d *DB) GetRunWebhookDeliveries(tx *sql.Tx, limit int) ([]*types.RunWebhookDelivery, error) {


This isn't really useful. It's used only by tests, you could use just a single functions with an optional filter on status, startSequence and limit. See also next comment.

sgotti · 2023-08-07T07:45:05Z

internal/services/notification/notification_test.go

+	t.Logf("starting webhooks client")
+
+	ns.c.WebhookURL = fmt.Sprintf("%s/%s", wr.exposedURL, "webhooks")
+	ns.c.WebhookSecret = "secretkey"


use a const

sgotti · 2023-08-07T07:45:30Z

internal/services/notification/notification_test.go

+			t.Fatalf("expected %s run webhook payload got: %s", runWebhooks[i].Payload, webhooks[i].payload)
+		}
+
+		h256 := hmac.New(sha256.New, []byte(ns.c.WebhookSecret))


use a const var, not ns.c.WebhookSecret

sgotti · 2023-08-07T07:59:19Z

services/notification/types/runwebhook.go

+type RunWebhook struct {
+	sqlg.ObjectMeta
+
+	Payload string `json:"payload,omitempty"`


no omitempty

sgotti · 2023-08-07T07:59:26Z

services/notification/types/runwebhookdelivery.go

+type RunWebhookDelivery struct {
+	sqlg.ObjectMeta
+
+	Sequence uint64


missing json tags

internal/services/notification/db/objects/objects.go

sgotti · 2023-08-07T08:02:41Z

services/notification/types/runwebhook.go

+type RunWebhook struct {
+	sqlg.ObjectMeta
+
+	Payload string `json:"payload,omitempty"`


sgotti · 2023-08-07T08:03:09Z

internal/services/notification/webhooksreceiver.go

+}
+
+type webhook struct {
+	payload   string


sgotti · 2023-08-07T08:26:23Z

internal/services/notification/db/objects/objects.go

+			{Name: "DeliveryStatus", Type: "types.DeliveryStatus", BaseType: "string"},
+			{Name: "DeliveredAt", Type: "time.Time", Nullable: true},
+			{Name: "Duration", Type: "time.Duration", Nullable: true},
+			{Name: "Status", Type: "string"},


Is Status really needed? Isn't StatusCode enough?

sgotti · 2023-08-08T13:44:37Z

internal/services/notification/db/objects/objects.go

+			{Name: "RunWebhookID", Type: "string"},
+			{Name: "DeliveryStatus", Type: "types.DeliveryStatus", BaseType: "string"},
+			{Name: "DeliveredAt", Type: "time.Time", Nullable: true},
+			{Name: "Duration", Type: "time.Duration", Nullable: true},


Duration is not really needed.

sgotti · 2023-08-08T13:45:02Z

internal/services/notification/runwebhookdelivery.go

+}
+
+func (n *NotificationService) sendRunWebhook(ctx context.Context, webhookPayload []byte, runWebhookUUID string) (*http.Response, *time.Duration, error) {
+	c := &http.Client{}


Why a new empty http client?

sgotti · 2023-08-08T13:47:02Z

internal/services/notification/db/tests/fixtures/migrate/dbv1.jsonc

This should contain some data for TestCreate.

sgotti · 2023-08-08T13:48:26Z

internal/services/runservice/db/tests/fixtures/migrate/dbv2.jsonc

This file is not related to this PR.

sgotti · 2023-08-08T15:57:04Z

internal/services/notification/runwebhookdelivery.go

+	}
+}
+
+func (n *NotificationService) handleRunWebhookDelivery(ctx context.Context, runWebhookDelivery *types.RunWebhookDelivery) error {


pass runWebhookDeliveryID

internal/services/notification/runwebhookdelivery.go

sgotti · 2023-08-09T12:23:30Z

internal/services/notification/runwebhookdelivery.go

+		}
+
+		if runWebhookDelivery.DeliveryStatus != types.DeliveryStatusNotDelivered {
+			return errors.Errorf("run webhook delivery %q already delivered", runWebhookDeliveryID)


No need to error, just return

sgotti · 2023-08-09T12:23:55Z

internal/services/notification/runwebhookdelivery.go

+			return errors.WithStack(err)
+		}
+		if runWebhookDelivery.DeliveryStatus != types.DeliveryStatusNotDelivered {
+			return errors.Errorf("run webhook delivery %q already delivered", runWebhookDeliveryID)


No need to error, just return.

internal/services/notification/webhooksreceiver.go

sgotti

Please fix the commit title (as the PR title that I have updated) and description (why notification: at every line?).

add db schema add RunWebhook and RunWebhookDelivery type add header X-Agola-Delivery when send run webhooks

alessandro-sorint · 2023-08-09T15:47:04Z

Please fix the commit title (as the PR title that I have updated) and description (why notification: at every line?).

@sgotti I fixed the commit

alessandro-sorint force-pushed the last-event branch 6 times, most recently from 1a7883b to a76f6e1 Compare July 10, 2023 13:23

alessandro-sorint changed the title ~~add ability to save run webhooks and update status delivery~~ add retry logic for webhook delivery Jul 10, 2023

alessandro-sorint force-pushed the last-event branch from a76f6e1 to ee79970 Compare July 14, 2023 08:22

alessandro-sorint force-pushed the last-event branch from ee79970 to a418cb4 Compare July 25, 2023 13:18

sgotti requested changes Jul 27, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch from a418cb4 to b757e04 Compare July 28, 2023 10:19

alessandro-sorint requested a review from sgotti July 31, 2023 07:00

alessandro-sorint force-pushed the last-event branch 2 times, most recently from ce21951 to ce4203d Compare August 2, 2023 07:31

alessandro-sorint changed the title ~~add retry logic for webhook delivery~~ adds saving of webhooks and delivery Aug 2, 2023

alessandro-sorint changed the title ~~adds saving of webhooks and delivery~~ add saving of webhooks and delivery Aug 2, 2023

alessandro-sorint force-pushed the last-event branch 2 times, most recently from dc68b82 to 02695bd Compare August 3, 2023 07:09

sgotti requested changes Aug 3, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch 2 times, most recently from bebb0ac to e5f9b94 Compare August 3, 2023 09:29

sgotti requested changes Aug 3, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch from e5f9b94 to b5524cc Compare August 3, 2023 10:23

alessandro-sorint requested a review from sgotti August 3, 2023 10:24

sgotti requested changes Aug 3, 2023

View reviewed changes

sgotti changed the title ~~add saving of webhooks and delivery~~ notification: store webhook and delivery Aug 3, 2023

sgotti requested changes Aug 3, 2023

View reviewed changes

internal/services/notification/runwebhookdelivery.go Outdated Show resolved Hide resolved

alessandro-sorint force-pushed the last-event branch from b5524cc to 19b493d Compare August 3, 2023 13:26

sgotti requested changes Aug 7, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch from d2f067d to 2ececfa Compare August 8, 2023 11:19

alessandro-sorint requested a review from sgotti August 8, 2023 11:48

sgotti requested changes Aug 8, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch from 2ececfa to 6ef35a1 Compare August 8, 2023 15:09

alessandro-sorint requested a review from sgotti August 8, 2023 15:24

sgotti requested changes Aug 8, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch 2 times, most recently from 31426dc to 7abafed Compare August 9, 2023 07:35

alessandro-sorint requested a review from sgotti August 9, 2023 07:56

alessandro-sorint force-pushed the last-event branch from 7abafed to d48fff4 Compare August 9, 2023 09:38

sgotti requested changes Aug 9, 2023

View reviewed changes

alessandro-sorint force-pushed the last-event branch from d48fff4 to b111876 Compare August 9, 2023 13:03

alessandro-sorint requested a review from sgotti August 9, 2023 13:18

sgotti requested changes Aug 9, 2023

View reviewed changes

internal/services/notification/webhooksreceiver.go Outdated Show resolved Hide resolved

alessandro-sorint force-pushed the last-event branch 2 times, most recently from df88a58 to 966169e Compare August 9, 2023 14:08

alessandro-sorint requested a review from sgotti August 9, 2023 14:08

sgotti reviewed Aug 9, 2023

View reviewed changes

internal/services/notification/webhooksreceiver.go Outdated Show resolved Hide resolved

alessandro-sorint force-pushed the last-event branch from 966169e to d11006f Compare August 9, 2023 14:51

alessandro-sorint requested a review from sgotti August 9, 2023 15:07

sgotti approved these changes Aug 9, 2023

View reviewed changes

sgotti changed the title ~~notification: store webhook and delivery~~ notification: store webhook and webhook delivery Aug 9, 2023

sgotti requested changes Aug 9, 2023

View reviewed changes

notification: store webhook and webhook delivery

ee0f10e

add db schema add RunWebhook and RunWebhookDelivery type add header X-Agola-Delivery when send run webhooks

alessandro-sorint force-pushed the last-event branch from d11006f to ee0f10e Compare August 9, 2023 15:45

sgotti approved these changes Aug 9, 2023

View reviewed changes

sgotti merged commit 7d3f59c into agola-io:master Aug 10, 2023
1 check passed

alessandro-sorint deleted the last-event branch November 20, 2023 08:36


		t.Logf("starting webhooks client")

		time.Sleep(1 * time.Second)

notification: store webhook and webhook delivery #413

notification: store webhook and webhook delivery #413

Conversation

alessandro-sorint commented Jul 3, 2023 • edited

alessandro-sorint commented Jul 26, 2023

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgotti left a comment

Choose a reason for hiding this comment

alessandro-sorint commented Aug 9, 2023

alessandro-sorint commented Jul 3, 2023 •

edited