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 Togglz provider #415

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

liran2000
Copy link
Member

Add Togglz provider.

Related Issues

Togglz issue.

Notes

  • Unofficial provider implementation for Togglz.
  • Tested by unit test using Togglz functionalities.

Signed-off-by: liran2000 <[email protected]>
@liran2000 liran2000 requested a review from a team as a code owner September 5, 2023 16:52
@liran2000
Copy link
Member Author

Hi @bennetelli, @hennr,

you can have a look and share your thoughts.

Thanks

boolean featureBooleanValue = feature.isActive();
return ProviderEvaluation.<Boolean>builder()
.value(featureBooleanValue)
.reason(Reason.TARGETING_MATCH.name())
Copy link
Member

Choose a reason for hiding this comment

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

You may want to use the reason STATIC since it doesn't look like evaluation context is being used.

https://openfeature.dev/specification/types#resolution-details

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure that STATIC is correct here, since Activation can be done by user even without passing context if I understand correctly.
@bennetelli, @hennr can share their thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you need to include FeatureUser when calling isActive?

Copy link
Member

@toddbaert toddbaert Sep 11, 2023

Choose a reason for hiding this comment

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

@beeme1mr I don't think so. It seems like Togglz uses pre-configured features/beans to get these values contextually. For example, to get the user name you configure one of these: https://www.togglz.org/documentation/authentication, which take the user-name from the servlet context, etc.

This is actually configured in the feature (see the constructor) if I understand correctly.

I'm honestly not sure how to properly determine the reason with this provider.

# Unofficial Togglz OpenFeature Provider for Java

## Usage
See [TogglzProviderTest.java](./src/test/java/dev/openfeature/contrib/providers/togglz/TogglzProviderTest.java)
Copy link
Member

Choose a reason for hiding this comment

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

It can be addressed in a follow-up PR, but please add more information to the readme. Please include information such as install instructions, basic usage, limitations (only booleans are supported), etc.

@bennetelli
Copy link

@liran2000 will have a look at it tomorrow. Thanks for your PR :) it's has been on my list for a while as well

@toddbaert
Copy link
Member

toddbaert commented Sep 7, 2023

@liran2000 I'll give this a proper review soon from a general OpenFeature perspective.

In the meantime, could you add yourself (and anyone else relevant, maybe @bennetelli ?) to the component owners? We're trying to identify owners for all new components. It functions somewhat like CODEOWNERS but just adds you as assignee when these files change.

Signed-off-by: liran2000 <[email protected]>
@liran2000
Copy link
Member Author

hi @bennetelli, can you review this from Togglz point of view ?

DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
…ure#415)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@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.

4 participants