feat: make provider interface "stateless" #299
Conversation
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
toddbaert
requested review from
justinabrahms,
beeme1mr,
hairyhenderson,
bacherfl,
kinyoklion,
Kavindu-Dodan,
aepfli,
craigpastro,
lukas-reining,
odubajDT and
guidobrei
openfeature/event_executor_test.go
Outdated
| @@ -975,7 +988,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { | |||
| rsp <- e | |||
| } | |||
|
|
|||
| AddHandler(ProviderReady, &callback) | |||
| //AddHandler(ProviderReady, &callback) // TODO: hard to test, because callback is executed on handler registration | |||
There was a problem hiding this comment.
unfortunately, yes. The RDY callback comes from emitOnRegistration. Now that i cannot mock the state via the provider implementation the testing this becomes a bit harder.
openfeature/client.go
Outdated
| @@ -41,6 +41,7 @@ type Client struct { | |||
| metadata ClientMetadata | |||
| hooks []Hook | |||
| evaluationContext EvaluationContext | |||
| name string | |||
There was a problem hiding this comment.
| name string | |
| domain string |
name has been renamed to domain in the spec (see change for details). I think there's a few places we need to update param names, etc, but if we're adding new fields, prefer domain.
There was a problem hiding this comment.
yes, thx i will rename that one. For the other ocurrences. I just saw that the old/wrong terminology is used in quite a lot of places. Wdyt about if I just open another PR that contains all the renaming?
There was a problem hiding this comment.
You can open a PR or even just an issue. 🙏
openfeature/client_test.go
Outdated
| if err := SetProviderAndWait(provider); err != nil { | ||
| t.Fatalf("failed to set up provider: %v", err) | ||
| } | ||
| eventually(t, func() bool { |
There was a problem hiding this comment.
I think we can get rid of all the eventually's here, and in similar tests, since you are using SetProviderAndWait, which blocks. In fact, I think that will increase the quality of these tests.
I also think it might reduce test interference and increase certainly if you pass t.Name() into all GetClient calls as the domain: GetApiInstance().GetClient(t.Name()), so that we can be even more confident there's no test interference through the singleton.
What do you think?
There was a problem hiding this comment.
I think that's a good idea :) will do. And you are absolutely right, no need for eventually in a lot of places 👌
openfeature/client_test.go
Outdated
| // is in NOT_READY. | ||
| func TestRequirement_1_7_6(t *testing.T) { | ||
| defer t.Cleanup(initSingleton) | ||
| t.Skip("Test not yet implemented") |
There was a problem hiding this comment.
Did you mean to implement these? (1.7.6 - 1.7.8)?
It seems to me that the short-circuiting logic is missing for NOT_READY and FATAL, at least. I think you'd need to implement that in client.evaluate.
There was a problem hiding this comment.
yes, let's do that in a another PR
There was a problem hiding this comment.
I think you can complete these now, yes? Or would you rather leave them to another PR?
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
warber
force-pushed
the
provider-no-state
branch
from
9782969 to
b482e5e
Compare
When triggering an evaluation while the provider is in "not ready" or "fatal state" the sdk shall return an appropriate error. Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
warber
force-pushed
the
provider-no-state
branch
from
b482e5e to
9695ff6
Compare
openfeature/error.go
Outdated
| package openfeature | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| ) | ||
|
|
||
| // ProviderInitError represents an error that occurs during provider initialization. | ||
| type ProviderInitError struct { | ||
| ErrorCode ErrorCode // Field to store the specific error code | ||
| Message string // Custom error message | ||
| } | ||
|
|
||
| // Error implements the error interface for ProviderInitError. | ||
| func (e *ProviderInitError) Error() string { | ||
| return fmt.Sprintf("ProviderInitError: %s (code: %s)", e.Message, e.ErrorCode) | ||
| } | ||
|
|
||
| // ProviderNotReadyError signifies that an operation failed because the provider is in a NOT_READY state. | ||
| var ProviderNotReadyError = errors.New("provider not yet initialized") | ||
|
|
||
| // ProviderFatalError signifies that an operation failed because the provider is in a FATAL state. | ||
| var ProviderFatalError = errors.New("provider is in an irrecoverable error state") |
There was a problem hiding this comment.
I think these contents should be moved to resolution_error.go where we already have some error consts and types.
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
There was a problem hiding this comment.
Hey @warber ! (sorry about the wait, been quite busy at KubeCon). This looks great to me now. I will merge this by the middle of next week unless I hear other objections from the TC or maintainers.
cc @kinyoklion @lukas-reining @thomaspoignant @justinabrahms
This PR
State management for providers is now handled internally, eliminating the need for providers to implement or maintain their own state. The associated method has been removed from the interface, simplifying provider implementations.
State updates occur both during provider initialization and dynamically at runtime through various events emitted by the provider implementation.
Related Issues
fixes #258