Skip to content
This repository has been archived by the owner on Aug 18, 2023. It is now read-only.

Refactor teamsNotifier, emailNotifier #37

Open
atc0005 opened this issue Apr 23, 2020 · 4 comments
Open

Refactor teamsNotifier, emailNotifier #37

atc0005 opened this issue Apr 23, 2020 · 4 comments
Assignees
Milestone

Comments

@atc0005
Copy link
Owner

atc0005 commented Apr 23, 2020

Both of these functions are nearly identical with the exception of the parameter list (just one?) and the anonymous function called within to perform the actual notification.

One route I tinkered with was creating a common settings "bundle" within the notifications manager and passing that bundle into two instances of a common notifier function. Another approach is to have only a single instance, but apply checks within to determine if notification type should be attempted.

Regardless of the final outcome, the initial implementation on the bench for #21 duplicates nearly 1:1 the content of teamsNotifier and emailNotifier functions and this is a strong code smell.

@atc0005
Copy link
Owner Author

atc0005 commented Apr 23, 2020

Thought: Create an actual Go type for each notification type (e.g., email, teams, ...)?

@atc0005
Copy link
Owner Author

atc0005 commented Apr 23, 2020

The timeoutValue could be calculated outside of the worker and passed to the specific notifier instance. This would be another example of keeping config package references concentrated within the StartNotifyMgr function.

timeoutValue := config.GetTimeout(
	sendTimeout,
	nextScheduledNotification,
	retries,
	retriesDelay,
)

The timeout, delays, etc currently unique to a specific notifier could also be handled in a similar manner.

@atc0005
Copy link
Owner Author

atc0005 commented Apr 23, 2020

Hearing about how database drivers "register" with the sql package via init side-effects makes me think about using a registration system here. It could help with a number of checks that are currently wired in that look to see whether notifications are enabled.

Example:

// If we don't have *any* notifications enabled we will just
// discard the item we have pulled from the channel
if !cfg.NotifyEmail() && !cfg.NotifyTeams() {
	// ...
}

@atc0005
Copy link
Owner Author

atc0005 commented May 17, 2020

see also atc0005/brick#4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant