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!: use (and require) OpenFeature SDK v2 #262

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Conversation

toddbaert
Copy link
Member

This PR updates the entire monorepo to use the 2.0+ version of the SDK, and for all the artifacts to require it.

There was no substantial changes besides the expected method renames, etc.

@toddbaert toddbaert requested review from a team as code owners August 21, 2024 17:54
@toddbaert toddbaert changed the title feat!: use sdk v2 and absorb changes feat!: use (and require) OpenFeature SDK v2 Aug 21, 2024
Comment on lines -88 to -93
/// <inheritdoc/>
public override ProviderStatus GetStatus()
{
return _status;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed (and e2e tests still pass) so everything is working as expected.

Comment on lines -104 to -106
public override ProviderStatus GetStatus()
public override async Task InitializeAsync(EvaluationContext context, CancellationToken cancellationToken = default)
{
return initialized ? ProviderStatus.Ready : ProviderStatus.NotReady;
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed.

{
return initialized ? ProviderStatus.Ready : ProviderStatus.NotReady;
await ServerDriver.Initialize();
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we just need to await this, no need to update any state.

@@ -65,7 +65,7 @@ public async Task GetValue_ForEnabledFeatureWithEvaluationContext_ReturnCorrectV
var date = DateTime.Now;
flags.GetFeatureValue("example-feature").Returns("true");
flags.IsFeatureEnabled("example-feature").Returns(true);
flagsmithClient.GetIdentityFlags("233", Arg.Is<List<ITrait>>(x => x.Count == 6 && x.Any(c => c.GetTraitKey() == "key1"))).Returns(flags);
flagsmithClient.GetIdentityFlags("233", Arg.Is<List<ITrait>>(x => x.Count > 6 && x.Any(c => c.GetTraitKey() == "key1"))).Returns(flags);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is perhaps the only slightly surprising change here. We include the targeting key in the props now, which makes the count here 1 larger than before.

<!-- 1.5-1.9999 -->
<OpenFeatureVer>[1.5,2.0)</OpenFeatureVer>
<!-- 2.0-2.9999 -->
<OpenFeatureVer>[2.0,3.0)</OpenFeatureVer>
Copy link
Member Author

@toddbaert toddbaert Aug 21, 2024

Choose a reason for hiding this comment

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

I've set this to only major version 2 up to 3 (exclusive).

{
return ResolveFlag(flagKey, context, defaultValue);
}

/// <inheritdoc/>
public override async Task<ResolutionDetails<Value>> ResolveStructureValue(string flagKey, Value defaultValue, EvaluationContext context = null)
public override async Task<ResolutionDetails<Value>> ResolveStructureValueAsync(string flagKey, Value defaultValue, EvaluationContext context = null, CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the CancellationToken is captured, we should pass it down to the GetValueDetailsAsync method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to really make any functional changes here, but this was a very easy one since the ConfigCat SDK already accepted a cancellation token, so I've done this.

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.