-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix: use the TargetingKey property in the Flagsmith provider #227
Conversation
LGTM - since we're on version 0.x still and this is now part of the specification, I would prefer making the breaking change now and removing the old targeting key behaviour. I'll leave this decision to the owners here. I'd also suggest updating the README's example to show how to use this, and fixing it since it doesn't work :) diff --git a/src/OpenFeature.Contrib.Providers.Flagsmith/README.md b/src/OpenFeature.Contrib.Providers.Flagsmith/README.md
index e8619e1..b1ae6f5 100644
--- a/src/OpenFeature.Contrib.Providers.Flagsmith/README.md
+++ b/src/OpenFeature.Contrib.Providers.Flagsmith/README.md
@@ -43,13 +43,16 @@ packet add OpenFeature.Contrib.Providers.Flagsmith
To create a Flagmith provider you should define provider and Flagsmith settings.
```csharp
-using OpenFeature.Contrib.Providers.Flagd;
+using OpenFeature.Contrib.Providers.Flagsmith;
+using OpenFeature.Model;
+using Flagsmith;
namespace OpenFeatureTestApp
{
- class Hello {
- static void Main(string[] args) {
-
+ class Hello
+ {
+ static async Task Main(string[] args)
+ {
// Additional configs for provider
var providerConfig = new FlagsmithProviderConfiguration();
@@ -63,14 +66,19 @@ namespace OpenFeatureTestApp
EnableAnalytics = false,
Retries = 1
};
- var flagsmithProvider = new FlagsmithProvider(providerConfig, flagsmithConfig);\
+ var flagsmithProvider = new FlagsmithProvider(providerConfig, flagsmithConfig);
// Set the flagsmithProvider as the provider for the OpenFeature SDK
- OpenFeature.Api.Instance.SetProvider(flagsmithProvider);
+ await OpenFeature.Api.Instance.SetProviderAsync(flagsmithProvider);
- var client = OpenFeature.Api.Instance.GetClient("my-app");
+ // Optional: set a targeting key and traits to use segment and/or identity overrides
+ var context = EvaluationContext.Builder()
+ .SetTargetingKey("my-flagsmith-identity-ID")
+ .Set("my-trait-key", "my-trait-value")
+ .Build();
- var val = client.GetBooleanValue("myBoolFlag", false, null);
+ var client = OpenFeature.Api.Instance.GetClient("my-app");
+ var val = client.GetBooleanValue("myBoolFlag", false, context);
// Print the value of the 'myBoolFlag' feature flag
System.Console.WriteLine(val.Result.ToString()); |
I can remove the old behaviour, I was just trying to avoid the breaking change as it is likely to affect all existing consumers. This could also be deprecated with What do you think @vpetrusevici @matthewelwell ? |
As @rolodato mentioned, since it's |
I have removed the old attribute and updated the documentation with a working example. |
{ | ||
key = value?.AsString; | ||
} | ||
var key = ctx?.TargetingKey; |
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.
Related: open-feature/dotnet-sdk#235
Though I think everything should be backwards compatible when we make this change.
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.
Approving but I'd like a signoff from a Flagsmith person!
Thanks @ghelyar !
Signed-off-by: ghelyar <3225358+ghelyar@users.noreply.github.com>
Signed-off-by: ghelyar <3225358+ghelyar@users.noreply.github.com>
Signed-off-by: ghelyar <3225358+ghelyar@users.noreply.github.com>
This PR
An explicit
TargetingKey
property was added to the OpenFeature SDK but the Flagsmith provider is not currently using it. This change uses the new property if it is set.