Refactored Subscription model #429
Conversation
Codecov Report
@@ Coverage Diff @@
## master #429 +/- ##
=========================================
- Coverage 64.49% 63.89% -0.6%
=========================================
Files 30 29 -1
Lines 1639 1601 -38
=========================================
- Hits 1057 1023 -34
+ Misses 538 536 -2
+ Partials 44 42 -2
Continue to review full report at Codecov.
|
| @@ -203,8 +203,9 @@ curl --request POST \ | |||
| --url http://localhost:4001/v1/spaces/default/subscriptions \ | |||
| --header 'content-type: application/json' \ | |||
| --data '{ | |||
| "type": "async", | |||
There was a problem hiding this comment.
Can we change this to something like "sync": "false". ATM, is easy to confuse type and eventType i think.
There was a problem hiding this comment.
I think that from UX perspective it's better to have it like that because it's more explicit what type the subscription is. It's easier to immediately understand what it actually do. Also it allows introducing more types in the future. @alexdebrie @plfx any thoughts?
In general we should discuss things like that on design phase.
There was a problem hiding this comment.
I'm good with type as originally designed
|
|
||
| `http` event is a built-in type of event occurring for HTTP requests on paths defined in HTTP subscriptions. The | ||
| `data` field of an `http` event has the following structure: | ||
| `http.request` event is a built-in type of event occurring for HTTP requests on paths defined in HTTP subscriptions. The |
There was a problem hiding this comment.
How about request.http so that we can use the same pattern in other types potentially, e.g. webhook.com.github.push
There was a problem hiding this comment.
I think that http.request actually follows the pattern proposed by the spec. Otherwise you would have push.github.com.
tests/integration_test.go
Outdated
| @@ -135,7 +137,8 @@ func TestIntegration_HTTPSubscription(t *testing.T) { | |||
|
|
|||
| postSubscription(testAPIServer.URL+"/v1/spaces/default/subscriptions", &subscription.Subscription{ | |||
| FunctionID: function.ID("httpresponse"), | |||
| Event: "http", | |||
| Type: subscription.TypeSync, | |||
| EventType: event.Type("http"), | |||
There was a problem hiding this comment.
Do you mean event.Type("http.request") or event.TypeHTTPRequest
There was a problem hiding this comment.
Good catch. Gonna fix that.
| @@ -119,18 +107,7 @@ func (service Service) DeleteSubscription(space string, id subscription.ID) erro | |||
| return &subscription.ErrSubscriptionNotFound{ID: sub.ID} | |||
| } | |||
|
|
|||
| if sub.Event == event.TypeHTTP { | |||
| err = service.deleteEndpoint(space, sub.Method, sub.Path) | |||
There was a problem hiding this comment.
Some some time ago there was a concept of endpoints in EG. We removed it from Config API but it was still in the logic to prevent creating conflicting HTTP endpoint. Right now I migrated that logic to use subscriptions (which we need regardless) and this model and storing this in etcd is useless.
How did you implement it:
New subscription model is the first part of refactoring that will simplify subscription, and in the same time make them more powerful.
This PR adds
typefiled in the subscription model. Subscription can beasyncorsync.eventfield has been renamed toeventType.httpevent type is replaced withhttp.requestevent type.Todos:
Is this ready for review?: YES
Is it a breaking change?: YES