refactor(notifications): isolate endpoint service #19818
Conversation
GeorgeMac
force-pushed
the
gm/notification-endpoints-service
branch
from
edf0d1b
to
2f236de
Compare
GeorgeMac
force-pushed
the
gm/notification-endpoints-service
branch
2 times, most recently
from
cdb3750
to
927262a
Compare
GeorgeMac
requested review from
stuartcarnie,
a team and
aanthony1243
and removed request for
stuartcarnie and
a team
GeorgeMac
force-pushed
the
gm/notification-endpoints-service
branch
from
927262a
to
f0c3c82
Compare
Following the ongoing effort to isolate behaviours into their own packages and off of kv.Service, this change move the notification endpoints service implementation into its own package. It removes the endpoint behaviors from the kv service completely.
GeorgeMac
force-pushed
the
gm/notification-endpoints-service
branch
from
f0c3c82
to
c94f478
Compare
There was a problem hiding this comment.
Looks good so far, @GeorgeMac – as you mentioned in the PR description, this is largely a lift of source from cloud, so I didn't thoroughly review those additions. A do see a few check / notification tests are still failing, so I'll leave those with you. Once those are passing, it has the green light from me 🚦
| @@ -367,10 +367,6 @@ func (tl *TestLauncher) BucketService(tb testing.TB) *http.BucketService { | |||
| return &http.BucketService{Client: tl.HTTPClient(tb)} | |||
| } | |||
|
|
|||
| func (tl *TestLauncher) CheckService() influxdb.CheckService { | |||
There was a problem hiding this comment.
Note for the reader:
You'll see this has moved up onto the Launcher for now. I have been moving more of these helpers to use the clients as opposed to in-process service implementations. I went to move this to the http.CheckService implementation. But half the methods don't actually match the influxdb.CheckService interface. They return their own local check API domain model and accept slightly different arguments.
I started down the path of fixing this client, but It started to get unwieldy. I still want to deliver that though as its own PR later. So I have made a note to create an issue and fix this.
Until then this just updates the launcher test suite to interface with the same check service installed within the launcher itself. So that consistency is at least achieved, even if it doesn't exercise the front door as we would like.
Closes #19799
This PR delivers a similar objective to #19804 in that it migrates a service behavior (notification endpoints) off of the
*kv.Serviceand isolates it into its own packagenotification/endpoint/service.A lot of this work has been delivered and validated in influx cloud. It is mostly a lift and drop of the implementation there, which in itself is a lift a drop of the existing
*kv.Serviveimplementation minus interactions with User Resource Mappings.This severs the dependency on user resource mapping service. This is further supporting the endeavor to stop using
*kv.Serviceas a tenant service: #19783Update:
I realize the new check service was never wired in either. As I have started to integrate parts of tenant in place of KV, checks service tests in
kvstarted failing. We have had a checks service defined which was never installed in the launcher. So instead of fixing a deprecated service. This PR now also installs thecheckspackage implementation instead. Which is already tested and works without URMs.