feat: flagd provider basic functionality #31

bacherfl · 2023-01-31T14:20:56Z

This PR builds upon the work that @benjiro did with regards to the Flagd implementation. In this PR, I added the Readme, added the remaining xml doc comments and added Unit tests.

Signed-off-by: Benjamin Evenson <[email protected]> Signed-off-by: Florian Bacher <[email protected]>

Signed-off-by: Florian Bacher <[email protected]>

toddbaert · 2023-02-01T19:57:07Z

I'll be looking at this today/tomorrow. Thanks @bacherfl !

Signed-off-by: Todd Baert <[email protected]>

toddbaert

wow @bacherfl and @benjiro ! Nice work. I think this looks good for a alpha release. I think the only thing I would like to see addressed in this PR would be changes to just throw in error conditions instead of manually returning the default - the SDK does that for us. Your tests look good to me as well. Quick question - did you manually test this as well with a small sample app of some kind? We will soon write integration tests using this provider in the dotnet SDK, so it will get tested rigorously eventually... I'm just wondering.

One other thing I will do is make sure to set this as a 0.0.1 release (if I don't do that, it will default to 1.0.0, which I'm sure we don't want at the moment). I can do this with changes to the release manifest, so don't worry and let me handle that part once we are otherwise set to merge! - this is already done.

The other things to do are:

automatically generate the grpc assets
add an LRU cache
automatic config via environment variables
Unix socket support
TLS support

I will create separate issues for these, and anyone can do them. Really great work, again!

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

toddbaert · 2023-02-02T20:51:38Z

src/OpenFeature.Contrib.Providers.Flagd/Proto/csharp/Schema.cs

+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: schema/v1/schema.proto
+// </auto-generated>


Unless you see some reason not to, I would .gitignore all this generated code and simply make sure the build reliably produces it. Again something we don't have to do now.

I agree, having this in the .gitignore definitely makes sense! I will try to get the files being generated on demand by the build and will let you know if I manage to do it in this PR, or if i need to do some more research + an extra PR to set this up in a more tidy way

alright, I think I managed to do it :) the Grpc client is now generated during the build, based on the .proto file in the imported submodule. @toddbaert do you know if in the build pipeline all git submodules are pulled when checking out the code? If not, i think this might need to be adapted to make the build + tests pass

I will add this!

I've done it.

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

toddbaert · 2023-02-02T21:13:50Z

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

+            }
+            catch (Grpc.Core.RpcException e)
+            {
+                return GetDefaultWithException<bool>(e, flagKey, defaultValue);


I would simplify the logic here a bit just to throw an informative error (you still make need to set the correct error code). The SDK itself will always handle any errors by defaulting. The most straightforward thing for a provider to do is simply throw an error with the correct code. https://github.com/open-feature/dotnet-sdk/blob/main/src/OpenFeature/Error/FeatureProviderException.cs

https://docs.openfeature.dev/docs/reference/concepts/provider#how-should-i-handle-error-conditions

This will be slightly easier when this is done, but you can do it now by just setting the right error type on the existing generic error class.

bacherfl · 2023-02-03T07:02:29Z

wow @bacherfl and @benjiro ! Nice work. I think this looks good for a alpha release. I think the only thing I would like to see addressed in this PR would be changes to just throw in error conditions instead of manually returning the default - the SDK does that for us. Your tests look good to me as well. Quick question - did you manually test this as well with a small sample app of some kind? We will soon write integration tests using this provider in the dotnet SDK, so it will get tested rigorously eventually... I'm just wondering.

One other thing I will do is make sure to set this as a 0.0.1 release (if I don't do that, it will default to 1.0.0, which I'm sure we don't want at the moment). I can do this with changes to the release manifest, so don't worry and let me handle that part once we are otherwise set to merge!

The other things to do are:

automatically generate the grpc assets

add an LRU cache

automatic config via environment variables

Unix socket support

TLS support

I will create separate issues for these, and anyone can do them. Really great work, again!

Hi @toddbaert thanks a lot for the review! I will address your comments as soon as possible and will keep you updated when I have pushed my changes! Regarding your question: I did do a quick manual test by running flagd locally and using the SDK with the Flagd provider registered (i.e. the example from the readme) - will do that again once I have made all adaptations to make sure everything works as expected :)

toddbaert · 2023-02-04T02:12:13Z

I've created 3 related issues. @bacherfl if you'd like to be assigned any of these, let me know.

#35
#36
#37

Signed-off-by: Florian Bacher <[email protected]>

…codes Signed-off-by: Florian Bacher <[email protected]>

Signed-off-by: Florian Bacher <[email protected]>

toddbaert · 2023-02-10T19:41:28Z

src/OpenFeature.Contrib.Providers.Flagd/OpenFeature.Contrib.Providers.Flagd.csproj

+    <PackageReference Include="Grpc.Tools" Version="2.51.0">
+      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>


Thanks for looking into this!

My only (minor) suggestion here is to move this and the ItemGroup element referencing the .proto file together, and add a little comment on how this generates the sources.

toddbaert · 2023-02-10T19:42:31Z

src/OpenFeature.Contrib.Providers.Flagd/README.md

+# Flagd Feature Flag .NET Provider

-Coming soon!
+The Flagd Flag provider allows you to connect to your Flagd instance.  
+
+# .Net SDK usage
+
+## Install dependencies
+
+The first things we will do is install the **Open Feature SDK** and the **GO Feature Flag provider**.
+
+### .NET Cli
+```shell
+dotnet add package OpenFeature.Contrib.Providers.Flagd
+```
+### Package Manager
+
+```shell
+NuGet\Install-Package OpenFeature.Contrib.Providers.Flagd
+```
+### Package Reference
+
+```xml
+<PackageReference Include="OpenFeature.Contrib.Providers.Flagd" />
+```
+### Packet cli
+
+```shell
+paket add OpenFeature.Contrib.Providers.Flagd
+```
+
+### Cake
+
+```shell
+// Install OpenFeature.Contrib.Providers.Flagd as a Cake Addin
+#addin nuget:?package=OpenFeature.Contrib.Providers.Flagd
+
+// Install OpenFeature.Contrib.Providers.Flagd as a Cake Tool
+#tool nuget:?package=OpenFeature.Contrib.Providers.Flagd
+```
+
+## Using the FlagdProvider with the OpenFeature SDK
+
+This example assumes that the flagd server is running locally
+For example, you can start flagd with the following example configuration:
+
+```shell
+flagd start --uri https://raw.githubusercontent.com/open-feature/flagd/main/config/samples/example_flags.json
+```
+
+When the flagd service is running, you can use the SDK with the FlagdProvider as in the following example console application:
+
+```csharp
+using OpenFeature.Contrib.Providers.Flagd;
+
+namespace OpenFeatureTestApp
+{
+    class Hello {
+        static void Main(string[] args) {
+            var flagdProvider = new FlagdProvider(new Uri("http://localhost:8013"));
+
+            // Set the flagdProvider as the provider for the OpenFeature SDK
+            OpenFeature.Api.Instance.SetProvider(flagdProvider);
+
+            var client = OpenFeature.Api.Instance.GetClient("my-app");
+
+            var val = client.GetBooleanValue("myBoolFlag", false, null);
+
+            // Print the value of the 'myBoolFlag' feature flag
+            System.Console.WriteLine(val.Result.ToString());
+        }
+    }
+}
+```
+
+### Configuring the FlagdProvider
+
+The URI of the flagd server to which the `FlagdProvider` connects to can either be passed directly to the constructor, or be configured using the following environment variables:
+
+| Option name           | Environment variable name       | Type    | Default   | Values        |
+| --------------------- | ------------------------------- | ------- | --------- | ------------- |
+| host                  | FLAGD_HOST                      | string  | localhost |               |
+| port                  | FLAGD_PORT                      | number  | 8013      |               |
+| tls                   | FLAGD_TLS                       | boolean | false     |               |
+
+So for example, if you would like to pass the URI directly, you can initialise it as follows:
+
+```csharp
+var flagdProvider = new FlagdProvider(new Uri("http://localhost:8013"));
+```
+
+Or, if you rely on the environment variables listed above, you can use the empty costructor:
+
+
+```csharp
+var flagdProvider = new FlagdProvider();
+```


Nothing to say except great work!

Signed-off-by: Todd Baert <[email protected]>

toddbaert · 2023-02-10T20:33:15Z

@bacherfl Amazing work! Everything seems to work great. I made very some small changes (and one suggestion above):

pulled in the submodule in the CI as you mentioned
added conditional compilation for non NETSTANDARD2_0 platforms (see commit, and here for more info)
ran dotnet format

I think we are done here 😎 . I can't thank you enough for your help.

toddbaert · 2023-02-10T20:43:25Z

src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs

+#if NETSTANDARD2_0
+            return new Service.ServiceClient(GrpcChannel.ForAddress(url));
+#else
+            return new Service.ServiceClient(GrpcChannel.ForAddress(url, new GrpcChannelOptions
+            {
+                HttpHandler = new WinHttpHandler()
+            }));
+#endif


@benjiro does this look right to you? It seems to pass in all the test scenarios now (462 was previously failing).

Signed-off-by: Florian Bacher <[email protected]>

bacherfl · 2023-02-13T06:40:24Z

@bacherfl Amazing work! Everything seems to work great. I made very some small changes (and one suggestion above):

pulled in the submodule in the CI as you mentioned

added conditional compilation for non NETSTANDARD2_0 platforms (see commit, and here for more info)

ran dotnet format

I think we are done here 😎 . I can't thank you enough for your help.

Thanks @toddbaert for the review and your help! I just pushed the suggested changes with regards to the .csproj file, so the PR should be good to go :)

feat: implement the flagd provider Signed-off-by: Florian Bacher <[email protected]> Co-authored-by: Benjamin Evenson <[email protected]> Co-authored-by: Todd Baert <[email protected]> Signed-off-by: Vladimir Petrusevici <[email protected]>

benjiro and others added 2 commits January 31, 2023 15:21

feat: implement the flagd provider

c092fa7

Signed-off-by: Benjamin Evenson <[email protected]> Signed-off-by: Florian Bacher <[email protected]>

added unit tests, xml doc and readme

aa8b6ca

Signed-off-by: Florian Bacher <[email protected]>

bacherfl force-pushed the feat/flagd-provider-implementation branch from 2bb8961 to aa8b6ca Compare January 31, 2023 14:21

Merge branch 'main' into feat/flagd-provider-implementation

e95d27e

beeme1mr requested a review from toddbaert January 31, 2023 14:28

bacherfl added 3 commits January 31, 2023 15:35

apply dotnet-format

a4411fb

Signed-off-by: Florian Bacher <[email protected]>

apply dotnet-format

a8ec6f1

Signed-off-by: Florian Bacher <[email protected]>

remove reference to WinHttpHandler

ff21785

Signed-off-by: Florian Bacher <[email protected]>

toddbaert mentioned this pull request Feb 1, 2023

feat: implement the flagd provider #23

Closed

fixup: make one constructor internal

b6cfdb1

Signed-off-by: Todd Baert <[email protected]>

toddbaert force-pushed the feat/flagd-provider-implementation branch from 47f7160 to b6cfdb1 Compare February 2, 2023 21:27

toddbaert reviewed Feb 2, 2023

View reviewed changes

toddbaert requested review from beeme1mr and benjiro February 2, 2023 21:43

This was referenced Feb 4, 2023

[flagd-provider] Implement Unix socket support #35

Closed

[flagd-provider] Implement TLS connection #36

Closed

[flagd-provider] Implement standard caching, with LRU cache #37

Closed

Implement cucumber integration test suite open-feature/dotnet-sdk#111

Closed

bacherfl added 5 commits February 8, 2023 10:28

added empty constructor to make use of environment variables

56b44db

Signed-off-by: Florian Bacher <[email protected]>

map exception to FeatureFlagProviderException with appropriate error …

b45b250

…codes Signed-off-by: Florian Bacher <[email protected]>

added generic Resolve function

c52f2b3

Signed-off-by: Florian Bacher <[email protected]>

generate grpc client during build, remove auto generated code from repo

96e305c

Signed-off-by: Florian Bacher <[email protected]>

Merge branch 'main' into feat/flagd-provider-implementation

f0cc7a0

bacherfl marked this pull request as ready for review February 9, 2023 15:42

toddbaert reviewed Feb 10, 2023

View reviewed changes

fixup: run dotnet format

9830a75

Signed-off-by: Todd Baert <[email protected]>

fixup: pull submodule in ci

27f592f

Signed-off-by: Todd Baert <[email protected]>

toddbaert force-pushed the feat/flagd-provider-implementation branch from d734bfc to 27f592f Compare February 10, 2023 19:52

toddbaert self-requested a review February 10, 2023 19:54

fixup: support 462

8877ada

Signed-off-by: Todd Baert <[email protected]>

toddbaert force-pushed the feat/flagd-provider-implementation branch from c7e13b1 to 8877ada Compare February 10, 2023 20:25

toddbaert approved these changes Feb 10, 2023

View reviewed changes

toddbaert reviewed Feb 10, 2023

View reviewed changes

incorporated suggestion for .csproj structure

ade7d01

Signed-off-by: Florian Bacher <[email protected]>

toddbaert changed the title ~~feat: flagd provider implementation~~ feat: flagd provider basic functionality Feb 13, 2023

toddbaert merged commit 5ed9336 into open-feature:main Feb 13, 2023

github-actions bot mentioned this pull request Feb 13, 2023

chore: release main #38

Merged

github-actions bot mentioned this pull request Sep 13, 2023

chore(main): release OpenFeature.Contrib.Providers.Flagd 0.1.7 #87

Merged

github-actions bot mentioned this pull request Apr 30, 2024

chore(main): release OpenFeature.Contrib.Providers.Flagd 0.2.0 #191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: flagd provider basic functionality #31

feat: flagd provider basic functionality #31

bacherfl commented Jan 31, 2023

toddbaert commented Feb 1, 2023

toddbaert left a comment •

edited

Loading

toddbaert Feb 2, 2023 •

edited

Loading

bacherfl Feb 8, 2023

bacherfl Feb 8, 2023

toddbaert Feb 10, 2023

toddbaert Feb 11, 2023

toddbaert Feb 2, 2023 •

edited

Loading

bacherfl commented Feb 3, 2023

toddbaert commented Feb 4, 2023

toddbaert Feb 10, 2023

toddbaert Feb 10, 2023

toddbaert commented Feb 10, 2023 •

edited

Loading

toddbaert Feb 10, 2023 •

edited

Loading

bacherfl commented Feb 13, 2023

		<PackageReference Include="Grpc.Tools" Version="2.51.0">
		<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

feat: flagd provider basic functionality #31

feat: flagd provider basic functionality #31

Conversation

bacherfl commented Jan 31, 2023

toddbaert commented Feb 1, 2023

toddbaert left a comment • edited Loading

Choose a reason for hiding this comment

toddbaert Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

bacherfl Feb 8, 2023

Choose a reason for hiding this comment

bacherfl Feb 8, 2023

Choose a reason for hiding this comment

toddbaert Feb 10, 2023

Choose a reason for hiding this comment

toddbaert Feb 11, 2023

Choose a reason for hiding this comment

toddbaert Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

bacherfl commented Feb 3, 2023

toddbaert commented Feb 4, 2023

toddbaert Feb 10, 2023

Choose a reason for hiding this comment

toddbaert Feb 10, 2023

Choose a reason for hiding this comment

toddbaert commented Feb 10, 2023 • edited Loading

toddbaert Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

bacherfl commented Feb 13, 2023

toddbaert left a comment •

edited

Loading

toddbaert Feb 2, 2023 •

edited

Loading

toddbaert Feb 2, 2023 •

edited

Loading

toddbaert commented Feb 10, 2023 •

edited

Loading

toddbaert Feb 10, 2023 •

edited

Loading