-
Notifications
You must be signed in to change notification settings - Fork 33
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: Blocking provider mutator #244
feat: Blocking provider mutator #244
Conversation
Signed-off-by: Mohamed VJ <muhammed8089@gmail.com> Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
Signed-off-by: Mohamed VJ <muhammed8089@gmail.com> Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
Signed-off-by: Mohamed VJ <muhammed8089@gmail.com> Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
95eefa2
to
b672d0c
Compare
Hi @toddbaert, could you please review this PR? I am very new to Go and not sure if this implementation is correct. I will refactor it and keep it DRY if the current implementation meets the expectations outlined in the issue. Thanks in advance. |
Thank you @znonthedev ! I will review, and also add some reviewers who are more experienced with Go than me! Many people are on holiday however, so it may take a few days to get some approvals. Your patience is appreciated! 🙏 |
This commit refactors the code by extracting the initialization and notification logic into a separate helper function called `initAndNotify()`. Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
==========================================
- Coverage 81.61% 79.83% -1.78%
==========================================
Files 10 10
Lines 1142 1195 +53
==========================================
+ Hits 932 954 +22
- Misses 192 220 +28
- Partials 18 21 +3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
I have updated the PR based on the comments. I have added two test cases also. I am not sure on the test cases. Do I need to add more test cases or change the current ones |
We need to add an equivalent function for setting a named provider and waiting (see here). |
@bacherfl when you get a moment, could you take a look at this as well? |
Certainly, @toddbaert. I had the same thought. I will incorporate the necessary changes into the PR |
…ization Signed-off-by: Mohamed V J <muhammed8089@gmail.com>
@@ -749,6 +749,87 @@ func TestRequirement_1_4_13(t *testing.T) { | |||
// Is tested by TestRequirement_4_4_2. | |||
|
|||
// TODO Requirement_1_6_1 | |||
|
|||
func TestWaitForProvider(t *testing.T) { |
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.
It might be nice to add a test that works like this:
- set a provider which is ready after 1s (has an init function that returns after 1s)
- returns a valid flag value after that time only
- returns an error before 1s
This is consistent with how we'd expect many providers to work. Maybe you could add some options to NewMockFeatureProvider
that would support this?
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.
okay, I got the idea. I'll give it a try, but since I'm new to Go, it might take me some time 😇 .
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 just had a look at the test, and to make sure the Init
function actually gets called, you will need to have a type that consolidates a MockFeatureProvider
and MockStateHandler
- otherwise the check here will fail and the function will proceed without calling the Init
function of the provider.
Since gomock, which we use for creating the mocks seems to not support the generation of a type that implements multiple interfaces, i believe the simplest way to get a struct that implements both the FeatureProvider and the StateHandler interfaces is to consolidate them into a single struct, such as follows:
type mockProviderAndStateHandler struct {
*MockFeatureProvider
*MockStateHandler
}
This type should be defined in the client_test.go
file, and can then be used to pass it to the SetProviderAndWait
function, such as in the following example:
...
mockProvider.EXPECT().Metadata().AnyTimes()
mockProvider.EXPECT().Hooks().AnyTimes()
mockProvider.EXPECT().IntEvaluation(context.Background(), flagKey, defaultValue, flatCtx).
Return(IntResolutionDetail{
Value: 0,
ProviderResolutionDetail: ProviderResolutionDetail{
ResolutionError: NewGeneralResolutionError("test"),
},
}).Times(2)
mockStateHandler := NewMockStateHandler(ctrl)
// TODO add the desired behavior for the Init function of the MockStateHandler
o := & mockProviderAndStateHandler{
MockFeatureProvider: mockProvider,
MockStateHandler: mockStateHandler,
}
err := SetProviderAndWait(o)
...
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.
please remove this file
// Operations happen concurrently. | ||
func (api *evaluationAPI) initNewAndShutdownOld(newProvider FeatureProvider, oldProvider FeatureProvider) { | ||
func (api *evaluationAPI) initNewAndShutdownOldAsync(newProvider FeatureProvider, oldProvider FeatureProvider) { |
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.
You may refactor to combine initNewAndShutdownOldAsync
& initNewAndShutdownOldSync
as they perform similar set of operations with only difference being sync vs async.
|
||
// setProviderAndWait sets the default FeatureProvider of the evaluationAPI. Returns an error if FeatureProvider is nil | ||
// This is a blocking call and will wait for the provider to be ready | ||
func (api *evaluationAPI) setProviderAndWait(provider FeatureProvider) 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.
Nitpick: to stay consistent with the naming i would suggest to call this function setProviderSync
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.
Agreed. The spec doesn't dictate function names. I think your suggestion makes sense here.
} | ||
|
||
// Initialize new named provider and shutdown the old one | ||
// Provider update must be non-blocking, hence initialization & shutdown happens concurrently |
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 this comment does not apply for this function anymore (since this is the blocking variant) and can be removed
@znonthedev are you planning to complete this contribution soon ? There are pending changes in some providers which require this feature. |
Closed with #251 |
This PR
Adds setProviderAndWait method in the API that will set the provider synchronously
Related Issues
Fixes #240
Follow-up Tasks
add test scenarios
update readme