Skip to content
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

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

ghelyar
Copy link
Contributor

@ghelyar ghelyar commented Jul 5, 2024

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
Copy link

rolodato commented Jul 6, 2024

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
Copy link
Contributor Author

ghelyar commented Jul 8, 2024

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
Copy link
Member

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
Copy link
Contributor Author

ghelyar commented Jul 8, 2024

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

{
key = value?.AsString;
}
var key = ctx?.TargetingKey;
Copy link
Member

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.

@toddbaert toddbaert self-requested a review July 8, 2024 16:57
Copy link
Member

@toddbaert toddbaert left a 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 !

@toddbaert toddbaert merged commit 5e175c8 into open-feature:main Jul 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants