gateway: add api to get run webhook deliveries #452
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
2 times, most recently
from
fbb24a6
to
2dc532f
Compare
alessandro-sorint
changed the title
|
@sgotti can you do the review please? |
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
2dc532f
to
5edf002
Compare
alessandro-sorint
changed the title
sgotti
requested changes
Nov 8, 2023
services/notification/types/types.go
Outdated
| const ( | ||
| SortDirectionAsc SortDirection = "asc" | ||
| SortDirectionDesc SortDirection = "desc" | ||
| ) |
| @@ -69,3 +70,41 @@ func (d *DB) migrateV3(tx *sql.Tx) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (d *DB) migrateV4(tx *sql.Tx) error { | |||
| var dmlGeneric = "DELETE FROM runwebhookdelivery" | |||
There was a problem hiding this comment.
so the migration will delete all the current webhook data?
There was a problem hiding this comment.
Can you be more detailed?
Since the notification service changes are done in this development cycle could just drop all the notification service data. But if we follow this line we could just also remove all the notifications service migrations until the official next release (people using master should manually clean the notification service db).
Instead if we're doing migrations then we should correctly migrate.
There was a problem hiding this comment.
ok @sgotti we can do migration, I can get ProjectID from Payload and populate the new column using a temporary table.
| HasMore bool | ||
| } | ||
|
|
||
| func (h *ActionHandler) GetRunWebhookDeliveries(ctx context.Context, req *GetRunWebhookDeliveriesRequest) (*GetRunWebhooksDeliveriesResponse, error) { |
There was a problem hiding this comment.
It's runwebhook deliveries by project so the types and functions names should reflect this.
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
2 times, most recently
from
f31c5bb
to
3f7917d
Compare
sgotti
requested changes
Nov 9, 2023
| @@ -15,6 +15,14 @@ const ( | |||
| DeliveryStatusDeliveryError DeliveryStatus = "deliveryError" | |||
| ) | |||
|
|
|||
| func DeliveryStatusFromStringSlice(slice []string) []DeliveryStatus { | |||
There was a problem hiding this comment.
A test for this function is probably needed.
| @@ -0,0 +1,36 @@ | |||
| package action | |||
There was a problem hiding this comment.
Probably actionHandler shouldn't belong to this commit but to the commit that add the apis handlers.
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
3 times, most recently
from
896e291
to
89e2c9e
Compare
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
4 times, most recently
from
c90272a
to
587475c
Compare
sgotti
requested changes
Nov 13, 2023
There was a problem hiding this comment.
I reversed engineered this commit and discovered that this change was done because the previous payload had a missing final curly bracket. Right? If so fix this before other changes in a dedicated commit so it's documented.
There was a problem hiding this comment.
Yes I changed also dbv1, dbv3 and import jsonc
There was a problem hiding this comment.
Can you please detail the commit description explaining what has been changed since the base64 changes aren't self explaining?
There was a problem hiding this comment.
I changed the commit message (I looked better the json has an excess curly bracket and not missed)
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
3 times, most recently
from
3cc64be
to
ed43979
Compare
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
ed43979
to
f4f5f5b
Compare
sgotti
reviewed
Nov 14, 2023
| {"exportMeta":{"kind":"RunWebhook"},"id":"ce98dd37-b7fd-4d0a-a744-8e1545de3a6c","creationTime":"2023-04-03T12:07:16.839654667Z","updateTime":"2023-04-03T12:07:16.839654667Z","payload":"eyJwcm9qZWN0X2luZm8iOnsicHJvamVjdF9pZCI6IjU3NTg4ZmU0LWI3YjgtNDdiOS1hNzhmLTdkOGUwNGRlMjU0NiJ9LCJydW4iOnsiaWQiOiIzNTU4OTllMi0xZDE3LTQ0YjMtYjMyYS04NDczZDQyYjE1NTEiLCJyZWZfdHlwZSI6ImJyYW5jaCIsInJlZiI6InJlZnMvaGVhZHMvbWFzdGVyIiwibmFtZSI6InJ1bjAxIiwiY291bnRlciI6MSwicGhhc2UiOiJxdWV1ZWQiLCJyZXN1bHQiOiJ1bmtub3duIiwidGFza3MiOnsiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0Ijp7ImlkIjoiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0IiwibmFtZSI6InRhc2swMSIsInN0YXR1cyI6Im5vdHN0YXJ0ZWQiLCJzZXR1cF9zdGVwIjp7InBoYXNlIjoibm90c3RhcnRlZCJ9LCJzdGVwcyI6W3sicGhhc2UiOiJub3RzdGFydGVkIn1dfX0sImVucXVldWVfdGltZSI6IjIwMjMtMDctMDNUMTE6MTA6MjYuMzA2Njk1OTk5KzAyOjAwIn0sIlZlcnNpb24iOjF9fQ=="} | ||
| {"exportMeta":{"kind":"RunWebhook"},"id":"d07ec67f-7a81-48e7-9d94-a87d29ca6ca3","creationTime":"2023-04-03T12:07:16.84108568Z","updateTime":"2023-04-03T12:07:16.84108568Z","payload":"eyJwcm9qZWN0X2luZm8iOnsicHJvamVjdF9pZCI6IjU3NTg4ZmU0LWI3YjgtNDdiOS1hNzhmLTdkOGUwNGRlMjU0NiJ9LCJydW4iOnsiaWQiOiIzNTU4OTllMi0xZDE3LTQ0YjMtYjMyYS04NDczZDQyYjE1NTEiLCJyZWZfdHlwZSI6ImJyYW5jaCIsInJlZiI6InJlZnMvaGVhZHMvbWFzdGVyIiwibmFtZSI6InJ1bjAxIiwiY291bnRlciI6MSwicGhhc2UiOiJxdWV1ZWQiLCJyZXN1bHQiOiJ1bmtub3duIiwidGFza3MiOnsiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0Ijp7ImlkIjoiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0IiwibmFtZSI6InRhc2swMSIsInN0YXR1cyI6Im5vdHN0YXJ0ZWQiLCJzZXR1cF9zdGVwIjp7InBoYXNlIjoibm90c3RhcnRlZCJ9LCJzdGVwcyI6W3sicGhhc2UiOiJub3RzdGFydGVkIn1dfX0sImVucXVldWVfdGltZSI6IjIwMjMtMDctMDNUMTE6MTA6MjYuMzA2Njk1OTk5KzAyOjAwIn0sIlZlcnNpb24iOjF9fQ=="} | ||
| {"exportMeta":{"kind":"RunWebhook"},"id":"ed2599f1-3cfc-4d09-afaf-934e2a4e1515","creationTime":"2023-04-03T12:07:16.839229324Z","updateTime":"2023-04-03T12:07:16.839229324Z","payload":"eyJwcm9qZWN0X2luZm8iOnsicHJvamVjdF9pZCI6IjU3NTg4ZmU0LWI3YjgtNDdiOS1hNzhmLTdkOGUwNGRlMjU0NiJ9LCJydW4iOnsiaWQiOiIzNTU4OTllMi0xZDE3LTQ0YjMtYjMyYS04NDczZDQyYjE1NTEiLCJyZWZfdHlwZSI6ImJyYW5jaCIsInJlZiI6InJlZnMvaGVhZHMvbWFzdGVyIiwibmFtZSI6InJ1bjAxIiwiY291bnRlciI6MSwicGhhc2UiOiJxdWV1ZWQiLCJyZXN1bHQiOiJ1bmtub3duIiwidGFza3MiOnsiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0Ijp7ImlkIjoiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0IiwibmFtZSI6InRhc2swMSIsInN0YXR1cyI6Im5vdHN0YXJ0ZWQiLCJzZXR1cF9zdGVwIjp7InBoYXNlIjoibm90c3RhcnRlZCJ9LCJzdGVwcyI6W3sicGhhc2UiOiJub3RzdGFydGVkIn1dfX0sImVucXVldWVfdGltZSI6IjIwMjMtMDctMDNUMTE6MTA6MjYuMzA2Njk1OTk5KzAyOjAwIn0sIlZlcnNpb24iOjF9fQ=="} | ||
| {"exportMeta":{"kind":"RunWebhook"},"id":"0f324898-442d-477c-93df-eb2229b3f922","creationTime":"2023-04-03T12:07:11.837436311Z","updateTime":"2023-04-03T12:07:11.837436311Z","payload":"eyJwcm9qZWN0X2luZm8iOnsicHJvamVjdF9pZCI6IjU3NTg4ZmU0LWI3YjgtNDdiOS1hNzhmLTdkOGUwNGRlMjU0NiJ9LCJydW4iOnsiaWQiOiIzNTU4OTllMi0xZDE3LTQ0YjMtYjMyYS04NDczZDQyYjE1NTEiLCJyZWZfdHlwZSI6ImJyYW5jaCIsInJlZiI6InJlZnMvaGVhZHMvbWFzdGVyIiwibmFtZSI6InJ1bjAxIiwiY291bnRlciI6MSwicGhhc2UiOiJxdWV1ZWQiLCJyZXN1bHQiOiJ1bmtub3duIiwidGFza3MiOnsiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0Ijp7ImlkIjoiODhiNDBlNGEtNDY5MC00MGFkLWJlZGMtZmE3ZWE2ZWNlNmY0IiwibmFtZSI6InRhc2swMSIsInN0YXR1cyI6Im5vdHN0YXJ0ZWQiLCJzZXR1cF9zdGVwIjp7InBoYXNlIjoibm90c3RhcnRlZCJ9LCJzdGVwcyI6W3sicGhhc2UiOiJub3RzdGFydGVkIn1dfX0sImVucXVldWVfdGltZSI6IjIwMjMtMDctMDNUMTE6MTA6MjYuMzA2Njk1OTk5KzAyOjAwIn0sIlZlcnNpb24iOjF9"} |
There was a problem hiding this comment.
To be completely correct in the base64 json "Version" should be lowercase "version" as defined in the json struct tag.
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
f4f5f5b
to
95f016b
Compare
sgotti
requested changes
Nov 14, 2023
| var runWebookDeliveries []*types.RunWebhookDelivery | ||
| err := h.d.Do(ctx, func(tx *sql.Tx) error { | ||
| var err error | ||
| runWebookDeliveries, err = h.d.GetRunWebhookDeliveriesAfterSequence(tx, req.StartSequence, req.ProjectID, req.DeliveryStatusFilter, limit, req.SortDirection) |
| @@ -83,12 +83,30 @@ func mustSingleRow[T any](s []*T) (*T, error) { | |||
| return s[0], nil | |||
| } | |||
|
|
|||
| func (d *DB) GetRunWebhookDeliveriesAfterSequence(tx *sql.Tx, afterSequence uint64, deliveryStatus types.DeliveryStatus, limit int) ([]*types.RunWebhookDelivery, error) { | |||
| func (d *DB) GetRunWebhookDeliveriesAfterSequence(tx *sql.Tx, afterSequence uint64, projectID string, deliveryStatusFilter []types.DeliveryStatus, limit int, sortDirection types.SortDirection) ([]*types.RunWebhookDelivery, error) { | |||
There was a problem hiding this comment.
Use a new function to filter by project and use the old one for the global run webhooks (you could also remove deliveryStatus arg if not used.)
| q.Where(q.E("delivery_status", deliveryStatus)) | ||
|
|
||
| if projectID != "" { | ||
| q.Join("runwebhook", "runwebhook.id=runwebhookdelivery.run_webhook_id") |
| @@ -90,7 +92,7 @@ func TestRunWebhookDelivery(t *testing.T) { | |||
|
|
|||
| runWebhooks := make([]*types.RunWebhook, MaxRunWebhookDeliveriesQueryLimit+10) | |||
| for i := 0; i < len(runWebhooks); i++ { | |||
| runWebhooks[i] = createRunWebhook(t, ctx, ns) | |||
| runWebhooks[i] = createRunWebhook(t, ctx, ns, "projectidtest") | |||
There was a problem hiding this comment.
Use a const and call it "project01"
| } | ||
|
|
||
| for i := 0; i < len(runWebhooks); i++ { | ||
| runWebhooks[i] = createRunWebhook(t, ctx, ns, "projectidtest02") |
| t.Fatalf("unexpected 10 run webhook deliveries, got %d", len(aresp.RunWebhookDeliveries)) | ||
| } | ||
|
|
||
| // test get run webhook deliveries with deliverystatusfilter delivered |
There was a problem hiding this comment.
// test get run webhook deliveries with deliverystatusfilter = delivered
| t.Fatalf("unexpected 20 run webhook deliveries, got %d", len(aresp.RunWebhookDeliveries)) | ||
| } | ||
|
|
||
| // test get run webhook deliveries with cursor |
There was a problem hiding this comment.
There isn't any cursor concept in the notification service.
| func TestGetProjectRunWebhookDeliveries(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("test get run webhook deliveries", func(t *testing.T) { |
There was a problem hiding this comment.
Why don't you use the same logic (multiple t.Run) as the configstore tests?
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
95f016b
to
6b42d11
Compare
sgotti
reviewed
Nov 14, 2023
| @@ -161,7 +165,7 @@ func TestRunWebhookDelivery(t *testing.T) { | |||
|
|
|||
| time.Sleep(1 * time.Second) | |||
|
|
|||
| runWebhook := createRunWebhook(t, ctx, ns) | |||
| runWebhook := createRunWebhook(t, ctx, ns, "projectidtest") | |||
sgotti
reviewed
Nov 14, 2023
| } | ||
|
|
||
| // test wrong deliverystatus | ||
| deliverystatus = []string{"notDelivered", "delivered", "deliveryError", "baddeliverystatus"} |
sgotti
reviewed
Nov 14, 2023
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
2 times, most recently
from
aee1ea3
to
35ded95
Compare
sgotti
requested changes
Nov 15, 2023
sgotti
requested changes
Nov 15, 2023
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
35ded95
to
99f2270
Compare
sgotti
reviewed
Nov 15, 2023
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
2 times, most recently
from
49118b5
to
aae8651
Compare
Add web server to notification service.
fix payload fields since the json value has an excess curly bracket and Version must be lovercase.
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
aae8651
to
7e03486
Compare
sgotti
reviewed
Nov 16, 2023
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
7e03486
to
7d1ecdc
Compare
sgotti
reviewed
Nov 16, 2023
add ProjectID to RunWebhook type
add an api to get run webhook deliveries by project id.
add an api to get run webhook deliveries by project ref.
alessandro-sorint
force-pushed
the
api-runwebhookdeliveries
branch
from
7d1ecdc
to
5c40a55
Compare
sgotti
approved these changes
Nov 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds an api to get run webhook deliveries by project ref.
The RunWebhook type add ProjectID field.
fix part of #424