refactor(notification): move rule service into own package #19804

GeorgeMac · 2020-10-22T13:42:03Z

Closes #19798

This creates the separate (non-URM interaction) notification rule service implementation. It is mostly a cut/paste of the kv service as it is today. Without URM interaction. Which we have shown to not be necessary in order to operate properly.

This doesn't remove the original definition in *kv.Service yet.
That will be delivered in a later PR (likely in the larger #19783).

Update:

A few extra things had to be delivered to put a bow on this:

TL;DRs

Launcher helpers updated to use notification rule client>
Fix added to notification rule client for bug when finding rules by ID.
Task http client changed to match service definition interface.
Remove the notification rule functionality from kv service.
Added new but deprecated resource.OrgIDResolver to sever the responsibility from *kv.Service.

Exhaustive explanations:

Introducing the changed notification rule service to the launcher broke pkger tests. Why? Because it turned out pkger tests were using the launcher helpers and the launcher helpers were often referring directly to the embedded kv service instead of going through the client to the daemon instance itself. I updated the launcher helper to return the client.
When returning the client this caused a panic in pkger itself. Further investigation showed that the notification rule client FindNotificationRuleByID did not work. Because of some poor unmarshalling of the convoluted json format this endpoint produces, it accidentally failed to unmarshall the embedded rule, resulting in a nil rule and a nil error. Referring to this rule down the line caused a panic.
The launcher helper used the task http client type directly (not the service abstraction), because the task client was never actually implemented to match the task service (inconsistent with every other service). So I fixed it up to be consistent and return the domain types.
TL;DR Remove the notification rule functionality from kv service. The new rule service no longer exposes an embedded urm or org service, which breaks the old functional style tests in testing. So the best option was to just remove those definitions now we have ported them next to the service itself (in notification/rule/service package). Doing so left the kv service notification rule service without tests. Since this is deprecating that service to ultimately be removed in the coming week, I decided to just remove it as a part of this PR.
The *kv.Service depended on implementing all the different service interfaces in order to support org ID resolution for some other services (see FindOrgIDForResource). This has now been moved into own type.

Well-formatted commit messages
Rebased/mergeable
Tests pass
Feature flagged (if modified API)
Documentation updated or issue created (provide link to issue/pr)
Signed CLA (if not already signed)

gavincabbage

My only nitpick is the stuttering of ruleservice.NewRuleService but that's not worth worrying about, just personal preference 😄

stuartcarnie

This looks good to me. I am tempted to be devil's advocate are ponder if we should keep this code in the same package as Cloud, however, I then convince myself it is a fallacy to think this code will / should remain the same between the two systems.

Once the tests pass, I give it the 🆗

notification/rule/service/service.go

GeorgeMac · 2020-10-23T11:17:41Z

@stuartcarnie I agree. I should have noted this actually. Putting it in the same place leads to the following packages:

notification
    endpoint
notifications
    endpoints

Which I couldn't bring myself to do. So I moved them flat into the existing:

notification
    endpoint

But then this created a cycle in dependencies because the services I ported depend on kv and kv depends (in OSS) on these packages for the domain types.
Long story short 😂 I want to flatten this all later once kv definitions are removed. Since kv will no longer depend on these notification specific packages.

…vice

GeorgeMac · 2020-10-23T11:21:21Z

http/notification_rule.go

+	LastRunError    string                `json:"lastRunError,omitempty"`
+}
+
+func (resp *notificationRuleResponse) UnmarshalJSON(v []byte) (err error) {


Note for reader:
The unmarshalling here never worked. Leading this client to result in a nil NotificationRule and a nil error when positively finding a rule.

This was causing a panic when this client was used for pkger tests.

Great find!

… client

This also introduces the org id resolver type. Which is transplanted from the kv service. As this one function coupled all resource capabilities onto the kv service. Making removing these capabilities impossible. Moving this type out into its own package which depends on each service explicitly ensures we don't have one type which has to implement all the service contracts.

GeorgeMac · 2020-10-24T12:27:32Z

@stuartcarnie and @gavincabbage I updated this PR in a number of ways. See the update in the description. As I kept pulling threads I found a bunch of fun traps. Which the PR should now resolve.

stuartcarnie

Great updates George – I can appreciate this is an arduous process, but your work to eliminate the kv.Service is very valued and appreciated 💯

internal/resource/org_id.go

stuartcarnie · 2020-10-26T14:59:12Z

http/notification_rule.go

+	LastRunError    string                `json:"lastRunError,omitempty"`
+}
+
+func (resp *notificationRuleResponse) UnmarshalJSON(v []byte) (err error) {


Great find!

stuartcarnie · 2020-10-26T15:51:00Z

authorizer/urm.go

@@ -6,19 +6,19 @@ import (
 	"github.com/influxdata/influxdb/v2"
 )

-type OrganizationService interface {
+type OrgIDResolver interface {


I'm not objecting to the name, but throwing out OrgIDFinder as an alternative, in case you like it and the verb is FindResourceOrganizationID.

Good shout. I actually rename this again in two PRs time to just Resolver (in the Telegraf PR). But I will rename it Finder in that PR as it finds both org IDs and name 👍

cmd/influxd/launcher/launcher.go

refactor(notification): move rule service into own package

17484d2

GeorgeMac requested review from stuartcarnie and a team October 22, 2020 13:42

gavincabbage approved these changes Oct 22, 2020

View reviewed changes

stuartcarnie reviewed Oct 22, 2020

View reviewed changes

notification/rule/service/service.go Outdated Show resolved Hide resolved

chore(launcher): fix tests to use clients as opposed to direct kv ser…

289a6d1

…vice

GeorgeMac requested a review from a team as a code owner October 23, 2020 11:20

GeorgeMac commented Oct 23, 2020

View reviewed changes

GeorgeMac requested review from stuartcarnie and gavincabbage October 23, 2020 11:22

GeorgeMac added 2 commits October 23, 2020 14:54

chore(influx): update task cli to consume core domain model task from…

f375730

… client

GeorgeMac force-pushed the gm/notification-rules-service branch from c701836 to 17c9f8e Compare October 24, 2020 12:01

fix(launcher): remove double reference to influxdb package

764c8a5

GeorgeMac force-pushed the gm/notification-rules-service branch from 933463d to 764c8a5 Compare October 24, 2020 12:26

GeorgeMac mentioned this pull request Oct 26, 2020

refactor(notifications): isolate endpoint service #19818

Merged

4 tasks

stuartcarnie approved these changes Oct 26, 2020

View reviewed changes

GeorgeMac merged commit 3d643e0 into master Oct 27, 2020

GeorgeMac deleted the gm/notification-rules-service branch October 27, 2020 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(notification): move rule service into own package #19804

refactor(notification): move rule service into own package #19804

GeorgeMac commented Oct 22, 2020 •

edited

Loading

gavincabbage left a comment

stuartcarnie left a comment

GeorgeMac commented Oct 23, 2020

GeorgeMac Oct 23, 2020

GeorgeMac Oct 23, 2020

stuartcarnie Oct 26, 2020

GeorgeMac commented Oct 24, 2020

stuartcarnie left a comment

stuartcarnie Oct 26, 2020

stuartcarnie Oct 26, 2020

GeorgeMac Oct 26, 2020

refactor(notification): move rule service into own package #19804

refactor(notification): move rule service into own package #19804

Conversation

GeorgeMac commented Oct 22, 2020 • edited Loading

gavincabbage left a comment

Choose a reason for hiding this comment

stuartcarnie left a comment

Choose a reason for hiding this comment

GeorgeMac commented Oct 23, 2020

GeorgeMac Oct 23, 2020

Choose a reason for hiding this comment

GeorgeMac Oct 23, 2020

Choose a reason for hiding this comment

stuartcarnie Oct 26, 2020

Choose a reason for hiding this comment

GeorgeMac commented Oct 24, 2020

stuartcarnie left a comment

Choose a reason for hiding this comment

stuartcarnie Oct 26, 2020

Choose a reason for hiding this comment

stuartcarnie Oct 26, 2020

Choose a reason for hiding this comment

GeorgeMac Oct 26, 2020

Choose a reason for hiding this comment

GeorgeMac commented Oct 22, 2020 •

edited

Loading