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 Flipt provider #461

Merged
merged 20 commits into from
Oct 24, 2023
Merged

Conversation

liran2000
Copy link
Member

add Flipt provider.

See readme for details.

@markphelps you are welcome to review from Flipt perspective.

@toddbaert you are welcome to review from OpenFeature perspective.

Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@liran2000
Copy link
Member Author

@markphelps build passed locally but failing here since flipt-java is built with Java 11, and this job is build with Java 8.
Possibly flipt-java can have a Java 8 release, for example 0.2.11-java8 ?

Signed-off-by: liran2000 <liran2000@gmail.com>
@markphelps
Copy link
Contributor

@markphelps build passed locally but failing here since flipt-java is built with Java 11, and this job is build with Java 8. Possibly flipt-java can have a Java 8 release, for example 0.2.11-java8 ?

@liran2000 yeah I will try to setup a Java 8 release. Our Java SDK is actually autogenerated, so hopefully there arent any Java 9+ specific features (been a long time since I've written Java)

Signed-off-by: Liran M <77168114+liran2000@users.noreply.github.com>
@markphelps
Copy link
Contributor

@liran2000 I dont think it's going to work with Java 8: https://github.com/flipt-io/flipt-java/actions/runs/6355901633/job/17264700354

Looks like our SDK requires some functionality that isn't in 8 (Map.of)

@markphelps
Copy link
Contributor

is it possible to update this job to a more recent version of Java?

@liran2000
Copy link
Member Author

Looks like our SDK requires some functionality that isn't in 8 (Map.of)

I can try to take a look at Flipt SDK to see what is the amount of non compliant code.

is it possible to update this job to a more recent version of Java?

As a library, if it compiles with newer Java version it will break Java 8 compatability, and will require newer Java version from all usages.

@toddbaert
Copy link
Member

toddbaert commented Sep 29, 2023

is it possible to update this job to a more recent version of Java?

Lots of enterprises are (unfortunately) still using Java 1.8, which is why both the SDK and contribs target (and use) it.

@markphelps I suspect the biggest challenge will be getting your tooling/plugins/etc working with 8, though that's not strictly necessary. You should be able to target 1.8 and then you'll be only making code changes, which from 11 to 8 shouldn't be too huge 😬 ... but obviously it depends on your 11 language feature usage.

@liran2000
Copy link
Member Author

@markphelps opened PR for Flipt SDK.
There seems to be only the one mentioned code place to update, so changing the main branch and release seems simpler than a separate release for Java 8.
You can review, merge and release it if you agree for proceeding. Notice the release job itself has to compile with Java 8 too.
Thanks

Signed-off-by: Liran M <77168114+liran2000@users.noreply.github.com>
@markphelps
Copy link
Contributor

Hi @liran2000 ! We just released a new version which uses the sourceCompatibility / targetCompatibility features of gradle. Ive been told that this should be enough so that the JAR/class files should target Java 8.

Would it be possible to update the Flipt SDK to 0.2.13? https://central.sonatype.com/artifact/io.flipt/flipt-java ?

cc @dsinghvi

@liran2000
Copy link
Member Author

liran2000 commented Oct 11, 2023

Hi @liran2000 ! We just released a new version which uses the sourceCompatibility / targetCompatibility features of gradle. Ive been told that this should be enough so that the JAR/class files should target Java 8.

Would it be possible to update the Flipt SDK to 0.2.13? https://central.sonatype.com/artifact/io.flipt/flipt-java ?

cc @dsinghvi

Thanks for the effort, the output JAR classes are now "Java 8", I tried it, it failed since the source code still have Java 8+ code. Failure is in the test, which is like a runtime test:

dev.openfeature.contrib.providers.flipt.FliptProviderTest
Error: dev.openfeature.contrib.providers.flipt.FliptProviderTest -- Time elapsed: 1.066 s <<< ERROR!
java.lang.NoSuchMethodError: java.util.Map.of(Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/util/Map;

In the link provided, it is mentioned:

"Additionally, the source code cannot contain lambda expressions or any feature not available in Java 6."

I don't see how it can compile and run with Java 8 without removing the incompatible Map.of from source code.

@dsinghvi
Copy link

@liran2000 thanks for testing -- this was oversight on my part. Will rerelease a version of the generator today.

Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks great once the Flipt Java SDK is updated! thank you @liran2000 !!

liran2000 and others added 2 commits October 19, 2023 16:43
Co-authored-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
Signed-off-by: Liran M <77168114+liran2000@users.noreply.github.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@liran2000 liran2000 marked this pull request as ready for review October 19, 2023 13:53
@liran2000 liran2000 requested a review from a team as a code owner October 19, 2023 13:53
@beeme1mr beeme1mr requested a review from toddbaert October 23, 2023 17:28
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@toddbaert
Copy link
Member

I'm ready to approve once the above unresolved comments are resolved.

@liran2000
Copy link
Member Author

I'm ready to approve once the above unresolved comments are resolved.

Comments are addressed in recents commits, can you check latest code?

Signed-off-by: Liran M <77168114+liran2000@users.noreply.github.com>
@toddbaert
Copy link
Member

I'm ready to approve once the above unresolved comments are resolved.

Comments are addressed in recents commits, can you check latest code?

Oh I'm sorry, I kept following sent to old permalinks to previous commits! Yes I see they are resolved. Approving!

@toddbaert toddbaert self-requested a review October 24, 2023 18:43
@toddbaert toddbaert merged commit dee6529 into open-feature:main Oct 24, 2023
@liran2000 liran2000 deleted the feature/flipt branch October 24, 2023 18:55
@markphelps
Copy link
Contributor

Thank you @liran2000 for all your hard work on this!!! And thank you @toddbaert for the review. Will update our docs as well to mention this new provider

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

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.

5 participants