search

open-feature / ruby-sdk

Ruby implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
21 stars 7 forks source link

fix: Add domain to build_client #109

Closed grepfruit19 closed 4 months ago

grepfruit19 commented 5 months ago

This PR

Related Issues

Fixes #108

How to test

Instantiate the library with and without a domain for its provider.

grepfruit19 commented 4 months ago

@beeme1mr @maxveldink

I added some tests.

I didn't want to change the behavior of the library too much, but maybe trying to instantiate a client against a domain that doesn't exist should throw or something, I feel like instantiating with the NoOp provider is unexpected behavior.

maxveldink commented 4 months ago

Thanks for fixing this @grepfruit19! Definitely, an oversight I had when I was trying to convert us over from name to domain. I agree that we should completely drop name wherever it's used (pre-1.0, I don't think we should have the idea of "legacy" fields; we should attempt to match the spec as close as possible).

Further, I think dropping version makes sense as well. I think @technicalpickles or @josecolella could explain why it was added, but it's not a part of the current specification and might confuse users. One other thing to think through; I don't think we can reuse Metadata for both Providers and Clients; they have different required fields. I'd be up for either dropping that class in favor of a simple hash for now or introducing ClientMetadata and ProviderMetadata classes to explicitly state the keys we allow.

Let me know if that's enough to go on, or if you need more direction here. This will definitely be a breaking change.

grepfruit19 commented 4 months ago

Sounds good, I can add the changes to drop name and version if we're in agreement. I'd rather keep this PR limited in scope as far as adding the ClientMetadata and ProviderMetadata classes however, so I'd prefer not to add those changes in this PR.

grepfruit19 commented 4 months ago

@beeme1mr @maxveldink

I've removed name from the library, I think things should be good to go now. I've held off on removing version so that y'all can make a separate PR since it seems like that has some further discussion to be had.

Let me know if there's anything else to be done!

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 99.40%. Comparing base (e8fb8cc) to head (6d92a63).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #109 +/- ## ======================================= Coverage 99.39% 99.40% ======================================= Files 12 12 Lines 166 167 +1 ======================================= + Hits 165 166 +1 Misses 1 1 ```

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

grepfruit19 commented 4 months ago

@beeme1mr Is there a good way to check what provider a client is using then?

beeme1mr commented 4 months ago

@beeme1mr Is there a good way to check what provider a client is using then?

Hey @grepfruit19, you can use the provider metadata on the client. Here's how we implemented that in the JS SDK.

If you're interested, this is how we passed the provider reference to the client.

grepfruit19 commented 4 months ago

@beeme1mr Then in terms of this ticket, would you be okay with me un-exposing the provider and holding off on exposing the provider metadata for future work?

beeme1mr commented 4 months ago

@beeme1mr Then in terms of this ticket, would you be okay with me un-exposing the provider and holding off on exposing the provider metadata for future work?

Sure, no problem at all. Thanks for your work on this PR.

grepfruit19 commented 4 months ago

@beeme1mr I've updated the PR and commented out the corresponding tests

grepfruit19 commented 4 months ago

@beeme1mr I think @maxveldink is out of office today, if this looks good to you mind giving it an approve + merge?