open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
62 stars 17 forks source link

feat: Flag metadata #223

Closed askpt closed 7 months ago

askpt commented 7 months ago

Flag Metadata

Related Issues

Fixes #192

Notes

Follow-up Tasks

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (d792b32) 94.28% compared to head (03db18a) 94.47%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #223 +/- ## ========================================== + Coverage 94.28% 94.47% +0.19% ========================================== Files 25 27 +2 Lines 1067 1104 +37 Branches 115 119 +4 ========================================== + Hits 1006 1043 +37 Misses 37 37 Partials 24 24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

toddbaert commented 7 months ago

@askpt FlagMetadata and EventMetadata are shaped the same in the SDK:

https://openfeature.dev/specification/types#event-metadata https://openfeature.dev/specification/types#flag-metadata

I think for consistency, that means we should use the same type for these, which I guess would mean it should be a Dictionary<string, object>(), unless there's some way we can bring them in line with your new implementation in a non-breaking way... maybe with extension methods?

askpt commented 7 months ago

@askpt FlagMetadata and EventMetadata are shaped the same in the SDK:

https://openfeature.dev/specification/types#event-metadata https://openfeature.dev/specification/types#flag-metadata

I think for consistency, that means we should use the same type for these, which I guess would mean it should be a Dictionary<string, object>(), unless there's some way we can bring them in line with your new implementation in a non-breaking way... maybe with extension methods?

@toddbaert I added a TODO to change the dictionary to use the BaseMetadata type. This would be a breaking change. See https://github.com/open-feature/dotnet-sdk/pull/223/commits/5a2dcc9c3544c379f27f786dd9f4b6e2b66ab61f

toddbaert commented 7 months ago

I think the e2e tests are failing because of the change in the resolvers to include a new optional metadata param - I think the change in the signature is somehow causing an issue due to the circular dependency on flagd.

We've had an issue to remove this cycle for a while, as we have in other SDKs by adding the in-memory provider (which the spec requires anyway).

toddbaert commented 7 months ago

@askpt merged the recent changes to remove the flagd dep and now e2e looks good!