-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: Add config to allow surpressing notification on launch (flag cache load) #2534
feat: Add config to allow surpressing notification on launch (flag cache load) #2534
Conversation
…nOnInit is set to true
✅ Deploy Preview for go-feature-flag-doc-preview canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2534 +/- ##
==========================================
+ Coverage 85.39% 85.48% +0.09%
==========================================
Files 100 100
Lines 4436 4444 +8
==========================================
+ Hits 3788 3799 +11
+ Misses 518 516 -2
+ Partials 130 129 -1 ☔ View full report in Codecov by Sentry. |
This also aligns the interval with rest of the test code
Sorry about the repeated commits. I think I've fixed the issue, and hopefully it works now. |
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.
Thanks a lot for this PR @8ma10s this is really great.
I like your approach but I will tend to simplify it a bit by just adding some parameters rather than creating new functions that can be hard to understand what is happening.
Would it work for you too?
cmd/relayproxy/config/config.go
Outdated
// DisableNotificationOnInit (optional) set to true if you do not want to | ||
// send a notification when the flags are loaded. | ||
// This is useful if you do not want a Slack/Webhook notification saying that | ||
// the flags have been added every time you start the application. | ||
// Default is set to false for backward compatibility. | ||
// Default: false | ||
DisableNotificationOnInit bool `mapstructure:"disableNotificationOnInit" koanf:"disablenotificationoninit"` |
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.
While I understand why you've put notification
here, but I think it will be more explicit to use the name notifier
which is what we are using everywhere.
// DisableNotificationOnInit (optional) set to true if you do not want to | |
// send a notification when the flags are loaded. | |
// This is useful if you do not want a Slack/Webhook notification saying that | |
// the flags have been added every time you start the application. | |
// Default is set to false for backward compatibility. | |
// Default: false | |
DisableNotificationOnInit bool `mapstructure:"disableNotificationOnInit" koanf:"disablenotificationoninit"` | |
// DisableNotifierOnInit (optional) set to true if you do not want to | |
// send a notification when the flags are loaded. | |
// This is useful if you do not want a Slack/Webhook notification saying that | |
// the flags have been added every time you start the application. | |
// Default is set to false for backward compatibility. | |
// Default: false | |
DisableNotifierOnInit bool `mapstructure:"disableNotificationOnInit" koanf:"disablenotificationoninit"` |
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.
I think it makes sense, thank you. Yes, "notification" does seem a bit vague (as there can be other kinds of notifications), but "notifier" in this repo's context is pretty clear. Let me change that 👍
config.go
Outdated
// DisableNotificationOnInit (optional) set to true if you do not want to send | ||
// a notification when the flags are loaded. | ||
// This is useful if you do not want a Slack/Webhook notification saying that | ||
// the flags have been added every time you start the application. | ||
// Default is set to false for backward compatibility. | ||
// Default: false | ||
DisableNotificationOnInit bool |
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.
Same as before
// DisableNotificationOnInit (optional) set to true if you do not want to send | |
// a notification when the flags are loaded. | |
// This is useful if you do not want a Slack/Webhook notification saying that | |
// the flags have been added every time you start the application. | |
// Default is set to false for backward compatibility. | |
// Default: false | |
DisableNotificationOnInit bool | |
// DisableNotifierOnInit (optional) set to true if you do not want to send | |
// a notification when the flags are loaded. | |
// This is useful if you do not want a Slack/Webhook notification saying that | |
// the flags have been added every time you start the application. | |
// Default is set to false for backward compatibility. | |
// Default: false | |
DisableNotifierOnInit bool |
feature_flag.go
Outdated
} | ||
|
||
// retrieveFlagsAndInitializeCache is called when the feature flag is initialized | ||
func retrieveFlagsAndInitializeCache(config Config, cache cache.Manager, retrieverManager *retriever.Manager) error { |
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.
Rather having almost the same function for retrieveFlagsAndInitializeCache
and retrieveFlagsAndUpdateCache
I would have prefer to add a parameter to the previous function retrieveFlagsAndUpdateCache
and use the same code on both side.
Something like
func retrieveFlagsAndUpdateCache(config Config, cache cache.Manager, retrieverManager *retriever.Manager, isInitialization bool) error {
// ...
if isInitialization && config.DisableNotificationOnInit {
err = cache.UpdateCache(newFlags, config.internalLogger)
} else {
err = cache.UpdateCacheAndNotify(newFlags, config.internalLogger)
}
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.
Same deal as here: #2534 (comment)
I think I just tend to prefer having explicit function/method names just to make it a bit more readable on the caller code and avoid introducing too many parameters in a single method, but I think either works. To make it consistent with the code on the above comment, let me change this one as well 👍
internal/cache/cache_manager.go
Outdated
func (c *cacheManagerImpl) UpdateCache(newFlags map[string]dto.DTO, log *fflog.FFLogger) error { | ||
return c.updateCache(newFlags, log, false) | ||
} | ||
|
||
func (c *cacheManagerImpl) UpdateCacheAndNotify(newFlags map[string]dto.DTO, log *fflog.FFLogger) error { | ||
return c.updateCache(newFlags, log, true) | ||
} |
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.
Do we need those functions or should we just keep UpdateCache
with the new parameter?
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.
Conclusion
Sure, I think I can change the code to do that. Let me change my code.
Given that it's an internal interface that's not meant to be used by anything outside of this repo's code, I think we don't need to care so much for readability (or method clarity) as much as I initially thought.
My initial thought
I just thought that, given that CacheManager
is (technically) defined as an interface, it would be beneficial to have a separate method to clearly indicate what the method is doing on the inside, rather than having an argument that doesn't really convey anything to the reader of the code.
What I mean is, if I have a code that calls this method defined in the interface, it would be (slightly) easier for the readers to understand what's going on when written like
err = cache.UpdateCacheAndNotify(newFlags, config.internalLogger)
// do other stuff...
rather than
err = cache.UpdateCache(newFlags, config.internalLogger, true)
// do other stuff...
because the value of true
doesn't really convey anything just by reading it, whereas the method name UpdateCacheAndNotify
clearly tells that it's gonna "notify something".
… of `CacheManager`
… indicate initialization
|
||
err := cache.UpdateCache(newFlags, config.internalLogger) | ||
err = cache.UpdateCache(newFlags, config.internalLogger, isInit && !config.DisableNotifierOnInit) |
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.
Since we don't want to introduce breaking changes, we have to make false
to be the original behavior, so that the default behavior would be same as the behavior before this PR. This is why we're making the config value name to be disableNotiferOnInit
, and it's making this boolean logic slightly more difficult to read by introducing double negation.
Hopefully at some point, this repo makes the decision to make disabling the notifier the default behavior.
At that point, we can make the config value name to be enableNotifierOnInit
(without negation, and that should make this code much easier to read:
err = cache.UpdateCache(newFlags, config.internalLogger, isInit && config.EnableNotifierOnInit)
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.
Inverting the logic is something I am open with but we will need to explicitly guide GOFF users about this change in the future.
I'll keep it in mind in the potential breaking changes for the future.
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.
Thanks again, we are almost ready to go and to merge this PR.
I have just put some comments on the mocks because we can probably reuse the existing one instead of creating new ones.
|
||
err := cache.UpdateCache(newFlags, config.internalLogger) | ||
err = cache.UpdateCache(newFlags, config.internalLogger, isInit && !config.DisableNotifierOnInit) |
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.
Inverting the logic is something I am open with but we will need to explicitly guide GOFF users about this change in the future.
I'll keep it in mind in the potential breaking changes for the future.
Thanks a lot for your contribution @8ma10s this is really great. |
Quality Gate passedIssues Measures |
@thomaspoignant Thank you for reviewing and giving me suggestions! 🚀 |
Description
This pull request introduces a new feature to disable notifications on initialization and refactors the caching mechanism to support this feature. It also includes updates to the test suite to ensure the new functionality works as expected.
Problem
Resolution
DisableNotificationOnInit
configuration key. If set to true, avoid sending the notification on cache initialization (it will still send notification when the change is detected AFTER initialization)How to test
go test ./...
ffClient
should do the trick too.Closes issue(s)
Resolve #2532
Checklist
README.md
and/website/docs
)