Refactor Notification Engine for Escalations #9
Conversation
This commit refactors the notification engine to its onw package. Having the NE in it's own package will simplify further changes to it and ease the implementation of escalation plans.
dominikschulz
changed the title
| neConfig.SMTP.Sender = c.Config.Integrations.SMTP.Sender | ||
| neConfig.Service.ClickURLBase = c.Config.Service.ClickURLBase | ||
| neConfig.Service.CallbackURLBase = c.Config.Service.CallbackURLBase | ||
| c.Notify = ne.New(neConfig) |
There was a problem hiding this comment.
Is there a reason you don't just pass c.Config to ne.New()? My brain is tired but all of these copies look awkward.
There was a problem hiding this comment.
This is not very clever, indeed. Passing c.Config would force me to include the parent package (main) in a subpackage, which I try to avoid. But I`ll have to figure out a better way to handle the config settings here before this gets merged. I was thinking about either factoring the config out to its own package which can be included in chicklittle main and its subpackages or using interfaces.
|
Very interesting. I need some time to digest this. I really like how you've added interfaces. They feel much more Go-like than what was there before. I also like how you've moved the enqueueing/dequeueing logic out of the API and into helper functions. This improves readability significantly! One comment on the breaking out of the notification engine into it's own package: I think the 'ne' name isn't very descriptive. I think 'notification' would be a more obvious choice. Thoughts? Looks great. Once I sort through it and digest, I think we can definitely merge. I do think we should be doing all of the team notification work in its own branch and move to master when it's finally complete. I regret merging the incomplete, non-functional API to master and there are too many other bug fixes in there now to go and rip it out. Let's merge this into its own branch and continue our team notification development there. Awesome work!!! |
| NIP NotificationsInProgress | ||
| planChan = make(chan *NotificationRequest) | ||
| cfgFile *string | ||
| c ChickenLittle | ||
| ) | ||
|
|
||
| type ChickenLittle struct { |
There was a problem hiding this comment.
I recently saw another project that used an object called "Service" to hold all of the shared data like we're doing here with the ChickenLittle struct. Once we get this branch merged, I want to go in and rename this struct to Service, because that makes more sense, e.g. Service.Config, Service.DB, etc.
|
Thanks for your positive feedback. If you´d like to merge this to a separate feature branch I´d be happy to rebase my changes to this branch and keep on working in the branch until you think it`s ready for the master branch. |
|
The latest commit addresses the config issue by moving the config to it´s own package and renames ne to notification. The drawback I see with naming the notification engine package |
|
Would you mind changing this PR to target the newly created teamescalation branch? I will merge it in there, then merge some of my additions from the teamrotation branch and we can start on the work needed to make escalation work. |
|
BTW, I really appreciate all of the work you've done so I've made you a collaborator so you can commit to this repo. Please do all feature development work in feature branches. I regret polluting master with the team API stuff and will probably rip that out soon in order to return master to only having the people, notification plan, and notification APIs. |
|
Thanks! I did rebase already, but I'll have to put in some more work until the code can be reviewed. |
This is my first attempt to implement the escalation logic for people and teams on a unified base for #2 .
To help with the implementation I've introduced some interfaces and moved the notification engine to it's own package. These changes are quite large and touch many parts of the project. Before moving on with tests, documentation and cleanup I'd first like to get some feedback if you'd feel comfortable merging these kind of changes.
Please do not merge this branch, yet.