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

feat: Add OpenFeature.Extensions.Hosting package #181

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

austindrenski
Copy link
Member

@austindrenski austindrenski commented Jan 16, 2024

Closes: #264


Related

TODO:

  • moar tests
    • evaluation context
      • IHostingEnvironment sample
    • hooks
    • providers:
      • none
      • one
      • multiple
  • xml docs
  • readme docs

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: Patch coverage is 65.78947% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 90.82%. Comparing base (11a0333) to head (bcf19c4).
Report is 7 commits behind head on main.

❗ Current head bcf19c4 differs from pull request most recent head eda289e. Consider uploading reports for the commit eda289e to get more accurate results

Files Patch % Lines
...Extensions.Hosting/OpenFeatureBuilderExtensions.cs 58.10% 29 Missing and 2 partials ⚠️
src/Shared/CallerArgumentExpressionAttribute.cs 0.00% 5 Missing ⚠️
src/Shared/Check.cs 66.66% 1 Missing and 1 partial ⚠️
...sions.Hosting/Internal/OpenFeatureHostedService.cs 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
- Coverage   95.40%   90.82%   -4.59%     
==========================================
  Files          27       29       +2     
  Lines        1111     1068      -43     
  Branches      120      113       -7     
==========================================
- Hits         1060      970      -90     
- Misses         34       70      +36     
- Partials       17       28      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@austindrenski
Copy link
Member Author

austindrenski commented Jan 17, 2024

FYI couple of known test failures:

  1. DCO is intentionally broken by a temp commit containing a squash of fix: Fix ArgumentOutOfRangeException for empty hooks #187 and fix: More robust shutdown/cleanup/reset #188. Once those PRs are merged, I'll rebase and elide the temp commit.
  2. dotnet-format is being weird about OpenFeatureHostedService and I'm pretty sure its just a sign that we need to migrate from the dotnet-format tool, to the new dotnet format that's baked into recent SDKs.
  3. flaky unit tests: pretty sure these are all related to the async stuff, so hoping they'll be less common after chore(deps): update dependency flagsmith to v5.3.0 dotnet-sdk-contrib#184 lands, but in the meantime please just re-run the test if its failed at the time of your review.

@austindrenski austindrenski marked this pull request as ready for review January 17, 2024 16:03
@austindrenski austindrenski requested a review from a team as a code owner January 17, 2024 16:03
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch from a9fd12e to 9821a5b Compare January 17, 2024 16:14
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch from 9821a5b to 2eb7335 Compare January 17, 2024 18:50
@austindrenski austindrenski self-assigned this Jan 18, 2024
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch 2 times, most recently from 828cebb to cf419bd Compare January 20, 2024 00:01
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch 7 times, most recently from 7fb289d to a2cde68 Compare January 27, 2024 20:08
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch 2 times, most recently from 66e10b1 to 2422211 Compare January 27, 2024 20:12
See: open-feature/dotnet-sdk-contrib#127

Signed-off-by: Austin Drenski <austin@austindrenski.io>
@austindrenski austindrenski force-pushed the add-open-feature-extensions-hosting branch from 2422211 to bcf19c4 Compare January 29, 2024 13:43
@askpt askpt added this to the 2.0 (breaking) milestone Apr 8, 2024
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
@askpt askpt marked this pull request as draft April 10, 2024 07:03
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
@askpt askpt self-assigned this Apr 15, 2024
README.md Outdated Show resolved Hide resolved
builder.Services.AddOpenFeature(static builder =>
{
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<FeatureProvider, SomeFeatureProvider>());
builder.TryAddOpenFeatureClient(SomeFeatureProvider.Name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do request scoped DI? It would be really great to include an example where you sent request specific context in the client. Since it's a slightly more advanced topic, perhaps it could be included in the evaluation context section later in the doc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it should work. Here's an example from .NET FeatureManagement.

https://github.com/microsoft/FeatureManagement-Dotnet?tab=readme-ov-file#using-httpcontext

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to time this merge closely with the release or move this to a separate PR that can be merged after the 2.0 release.

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
{
foreach (var provider in this._providers)
{
await this._api.SetProviderAsync(provider.GetMetadata().Name ?? string.Empty, provider).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the meta-data name for a provider is an appropriate name here. We probably need a way to make providers that encapsulates that data.

Simple example is using one provider with two different environments.

But more than that the provider name is a different domain of names. It is the domain of the provider implementation, where client names are in the domain of the application. An application developer needs to know and assign that name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

askpt added 7 commits May 14, 2024 17:29
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
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.

Introduce OpenFeature.Extensions.Hosting package
5 participants