Skip to content

Conversation

@justinabrahms
Copy link
Member

@justinabrahms justinabrahms commented Mar 15, 2023

This PR

This adds a new provider based on jsonlogic data format.

Related Issues

Refs open-feature/ofep#57

Follow-up Tasks

Haven't yet done the "convert into a value" yet, but this is a reasonable start.

How to test

The tests run. Feel free to provide additional test case suggestions.

@beeme1mr
Copy link
Member

Hey @justinabrahms, thanks for putting this together. It's great to see new providers like this added to the contribs repo. I'll let @toddbaert and/or @Kavindu-Dodan handle the full review (I'm not a Java person) but the basics look good.

By the way, flagd extends JSON logic to support deterministic splits. If that's ever a requirement here, please consider using the same flag configuration and hash algorithm.

https://github.com/open-feature/flagd/blob/main/docs/configuration/fractional_evaluation.md

@toddbaert
Copy link
Member

toddbaert commented Mar 16, 2023

Could you add a src/ dir under the inline-evaluating-provider dir and put the source and test files there, like in the other libs? I think as it stands this might cause some issues because it's not a standard maven project dir.

EDIT: Actually, after testing a bit I don't think your tests are running in the CI for this reason, and the package will probably contain no bytecode. I added a test that should have failed and it did not.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Looks good in theory, but I think you'll need to resolve this or no actual java bytecode will be published.

@toddbaert
Copy link
Member

toddbaert commented Mar 16, 2023

You may want to add an entry here, or this will default to 1.0 on release (up to you though).

@justinabrahms justinabrahms requested a review from a team as a code owner March 25, 2023 18:08
@justinabrahms justinabrahms force-pushed the inline-eval branch 2 times, most recently from c6d8ab9 to 81f70d3 Compare April 7, 2023 18:22
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
…ance.

Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

LGTM with the exception of the metadata as @thiyagu06 pointed out: #241 (comment)

Copy link
Collaborator

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

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

Approving with some comments :)

justinabrahms and others added 6 commits April 12, 2023 21:48
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms justinabrahms merged commit bba8ef3 into main Apr 13, 2023
@justinabrahms justinabrahms deleted the inline-eval branch April 13, 2023 12:26
DBlanchard88 pushed a commit to DBlanchard88/java-sdk-contrib that referenced this pull request Apr 29, 2024
…e#241)

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.

6 participants