Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
44a87b2
35e4274
60c158c
ec669de
f790787
ea65873
fa5dda7
ad3bf62
9b46412
f4f62dc
09852d0
b282acc
eac35b7
e39b7d1
85d7cb4
86d93e3
b064acb
2783fa1
d267719
ce61272
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 95 in cmd/relayproxy/service/gofeatureflag.go
Codecov / codecov/patch
cmd/relayproxy/service/gofeatureflag.go#L95
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 bedisableNotiferOnInit
, 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: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.