open-feature / dotnet-sdk-contrib

OpenFeature Providers and Hooks for .NET
https://openfeature.dev
Apache License 2.0
30 stars 22 forks source link

fix: use the TargetingKey property in the Flagsmith provider #227

Closed ghelyar closed 4 months ago

ghelyar commented 4 months ago

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.

rolodato commented 4 months ago

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());
ghelyar commented 4 months ago

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 [Obsolete] on IFlagsmithProviderConfiguration.TargetingKey, FlagsmithProviderConfiguration.TargetingKey and FlagsmithProviderConfiguration.DefaultTargetingKey first and then removed later, to make the migration a bit easier for consumers, although they might not be using those properties, they might just be using the string "targetingKey".

What do you think @vpetrusevici @matthewelwell ?

matthewelwell commented 4 months ago

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 [Obsolete] on IFlagsmithProviderConfiguration.TargetingKey, FlagsmithProviderConfiguration.TargetingKey and FlagsmithProviderConfiguration.DefaultTargetingKey first and then removed later, to make the migration a bit easier for consumers, although they might not be using those properties, they might just be using the string "targetingKey".

What do you think @vpetrusevici @matthewelwell ?

As @rolodato mentioned, since it's 0.x I'm happy to directly apply the breaking change.

ghelyar commented 4 months ago

I have removed the old attribute and updated the documentation with a working example.