feat: Add Harness provider #348
Conversation
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
There was a problem hiding this comment.
Hi @liran2000 thanks for putting this together. At a high level it looks good.
I'm just going to take one more look over it and then I'll approve.
We recently made a change in the goland SDK around initialisation which might help determine when the SDK is ready or not (once its connected and fetched all its config).
| harnessClient, err := harness.NewCfClient(p.providerConfig.SdkKey, p.providerConfig.Options...) | ||
| if err != nil { | ||
| p.status = of.ErrorState | ||
| } else { | ||
| p.status = of.ReadyState | ||
| p.harnessClient = harnessClient | ||
| } | ||
| return err |
There was a problem hiding this comment.
Does harness include any sort of change events? We don't need it for first release, but we could maybe make an issue for it.
There was a problem hiding this comment.
I didn't see events in Harness GO SDK which can be mapped to provider events. If so then it can be address as a follow-up issue yes.
There was a problem hiding this comment.
@liran2000 I think this is something we could add into the golang SDK. Currently we expose a func to tell you if it is initialised. We debated about exposing a channel to send the event on, but opted not to. I think what we can do is make it a option to pass into the constructor, where you can pass you're own channel in and get a single when we successfully init
There was a problem hiding this comment.
I think that would be a cool feature. Personally I've always found change events in SDKs useful.
providers/harness/pkg/provider.go
Outdated
| switch key { | ||
| case "Identifier": | ||
| harnessTarget.Identifier = val | ||
| case "Name": | ||
| harnessTarget.Name = val | ||
| default: |
There was a problem hiding this comment.
if Name or Identifier are special fields for identifying a target/user, we might want to map the targetingKey into one of these, and document which it is. We also may want to provide an option for which it's mapped to, if both are used for that purpose.
We also should specify what happens if targetingKey and one of these is set. I think targetingKey should take priority, since that is OpenFeatures designated identifier for a context. We may want to log a warning if that happens though.
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Thanks @davejohnston, I made some changes according to comments, you can have a look and write here if you approve. |
| Evaluation Context is mapped to Harness [target](https://developer.harness.io/docs/feature-flags/ff-sdks/server-sdks/feature-flag-sdks-go-application/#add-a-target). | ||
| OpenFeature targetingKey is mapped to _Identifier_, _Name_ is mapped to _Name_ and other fields are mapped to Attributes | ||
| fields. |
Add Harness GO Provider.
@davejohnston you are welcome to review from Harness point of view.
@Kavindu-Dodan @toddbaert you are welcome to review from OpenFeature perspective.