feat: Eventing support for flagd provider #317
Conversation
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
| @@ -12,7 +12,7 @@ test: | |||
| go list -f '{{.Dir}}/...' -m | xargs -I{} go test -v {} | |||
|
|
|||
| lint: | |||
| go install -v github.com/golangci/golangci-lint/cmd/golangci-lint@latest | |||
| go install -v github.com/golangci/golangci-lint/cmd/golangci-lint@v1.54.1 | |||
There was a problem hiding this comment.
the latest release is not compatible with go version 1.19+ variants
| if s.cache.IsEnabled() { | ||
| fromCache, ok := s.cache.GetCache().Get(key) | ||
| if ok { | ||
| fromCacheResDetail, ok := fromCache.(openfeature.BoolResolutionDetail) | ||
| if ok { | ||
| fromCacheResDetail.Reason = ReasonCached | ||
| return fromCacheResDetail | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be great if we could use generics or some other mechanism to reduce this duplication.
There was a problem hiding this comment.
+1 on this. I did not attempt this to reduce review efforts. Change in the PR fully maps to current main's logic (helps to see if there's API change/response change). But once merged, we might be able to find some commons and extract them to generic handlers
There was a problem hiding this comment.
I think this is the only blocker for my approval (but please correct me if I'm misunderstanding).
Somewhat related: I think we should (in a spearate PR) implement the e2e flagd tests we had in the go-sdk here (since we've replaced them with the in-memory provider). This would give us more confidence around things like dep updates, etc in this provider.
I would be happy to work on that, or at least create an issue.
Co-authored-by: Todd Baert <[email protected]> Signed-off-by: Kavindu Dodanduwa <[email protected]>
Co-authored-by: Todd Baert <[email protected]> Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
providers/flagd/pkg/configuration.go
Outdated
| } | ||
|
|
||
| certificatePath := os.Getenv(flagdServerCertPathEnvironmentVariableName) | ||
| if certificatePath != "" || os.Getenv(flagdTLSEnvironmentVariableName) == "true" { |
There was a problem hiding this comment.
maybe worth trimming and lowercase the value of flagdTLSEnvironmentVariableName
There was a problem hiding this comment.
not sure this is needed 🤔 I didn't want to add/change the behavior of this logic.
There was a problem hiding this comment.
Agree, this doesn't need to be part of this PR. However, maybe it is worth unifying across languages how to process bool env var. @toddbaert do we have somewhere in the spec a common way for SDKs to handle truthy and falsy values?
There was a problem hiding this comment.
We document the value in lowercase 1. May be this is sufficient
Footnotes
There was a problem hiding this comment.
And I updated the documentation to have lowercase options. See commit 0756c4f
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Kavindu Dodanduwa <[email protected]>
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Kavindu-Dodan
force-pushed
the
feat/flagd-eventing
branch
from
16fe631 to
0756c4f
Compare
This PR
Adds OpenFeature Events extension 1 support to flagd provider
tldr
IService.goas the main interface between provider and grpc handling (flagd connectivity)configuration.goReview commit by commit OR with a checkout of the original branch to have more focus
Main refactoring on this PR focuses on isolating grpc based flagd connectivity to the service layer. This was done by refactoring
provider.goand updatingIService.go. With the change, we no longer have grpc dependencies at provider level. This is favorable for future efforts such as in-memory provider capability proposed through open-feature/flagd#848Caching is now isolated to a dedicated service. This isolation helps to pass the service to appropriate caching locations and maintain a single cache within the provider.
Provider configurations were also extracted to a new component to reduce logic at the provider level.
Given below is an overview of component relationships in the current state,
Footnotes
https://openfeature.dev/specification/sections/events ↩