-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Notification Addition #73
Conversation
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 89.93% 87.97% -1.97%
==========================================
Files 11 11
Lines 497 524 +27
==========================================
+ Hits 447 461 +14
- Misses 30 36 +6
- Partials 20 27 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition. Just a few comments
config/config.go
Outdated
@@ -40,16 +40,18 @@ type Field struct { | |||
version uint64 | |||
structField CfgType | |||
sources map[Source]string | |||
chNotify chan<- string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the ability to track changes in configuration is awesome, but I think sending just a string doesn't give too much value.
It would be better to send a notification struct
with more information like the previous and next values along with the field (at least its name) so that some kind of calculation can be done from the user.
For example if there is a bigger change than x%
or maybe send these values to another service (ex. auditing, event message etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. On it.
addr: addr, | ||
dataCenter: dataCenter, | ||
token: token, | ||
timeout: timeout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement 🚀
Which problem is this PR solving?
Closes #17.
Short description of the changes