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.

Sign up for GitHub

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

Jump to bottom

feat: add Unleash provider #424

Merged
merged 20 commits into from
Sep 28, 2023
Merged

feat: add Unleash provider #424

merged 20 commits into from
Sep 28, 2023

Conversation

liran2000
Copy link
Member

add Unleash provider.

See Readme for details.

Discussed at Unleash.

@liran2000
Copy link
Member Author

liran2000 commented Sep 7, 2023 •
edited
Loading

hi @pransh15, @chriswk, @ivarconr, @gastonfournier,

you can have a look and share your thoughts.

Also to be reviewed from OpenFeature perspective.

gastonfournier
gastonfournier reviewed Sep 8, 2023
View reviewed changes
Copy link

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

This looks promising! I leave you a few comments to make it better, but I'm glad you're doing this!

It might be worthy adding a comment in https://github.com/orgs/Unleash/discussions/3896 to engage other contributors.

providers/unleash/src/main/java/dev/openfeature/contrib/providers/unleash/UnleashProvider.java Outdated Show resolved Hide resolved
.../src/test/java/dev/openfeature/contrib/providers/unleash/UnleashProviderIntegrationTest.java Outdated Show resolved Hide resolved
@toddbaert toddbaert self-requested a review September 8, 2023 14:39
@liran2000
Copy link
Member Author

liran2000 commented Sep 8, 2023 •
edited
Loading

This looks promising! I leave you a few comments to make it better, but I'm glad you're doing this!

It might be worthy adding a comment in https://github.com/orgs/Unleash/discussions/3896 to engage other contributors.

Thanks @gastonfournier !
I made some adjustments based on your comments.
Since you probably more familiar with Unleash, I am interested in opinions on the concepts of this provider as described at the Readme:

Concepts

  • Boolean evaluation gets feature enabled status.
  • String evaluation gets feature variant value.

@liran2000
- some fixes
5eef0e2 
- test using WireMock

Signed-off-by: liran2000 <[email protected]>
@liran2000
update WireMock version to match SDK Java version
f67742a 
Signed-off-by: liran2000 <[email protected]>
@liran2000
add shutdown method
92e3a62 
Signed-off-by: liran2000 <[email protected]>
@liran2000
tests updates
8a63167 
Signed-off-by: liran2000 <[email protected]>
@liran2000 liran2000 mentioned this pull request Sep 11, 2023
Closed
@liran2000
- focused test features
99dd6a7 
- update name
- object evaluation

Signed-off-by: liran2000 <[email protected]>
@liran2000
enhancements
8f93bbb 
- int,double evaluation
- flag metadata

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

hi @ivarconr what do you think about proceeding with this PR and adding Unleash provider here ?
basically most of the comments are addressed.

@liran2000 liran2000 marked this pull request as ready for review September 20, 2023 16:59
@liran2000 liran2000 requested a review from a team as a code owner September 20, 2023 16:59
@ivarconr
Copy link
Contributor

hi @ivarconr what do you think about proceeding with this PR and adding Unleash provider here ?
basically most of the comments are addressed.

All in favor. Would love to get to test it in an example app, either as a alpha release or if you provide snapshot versions.

@toddbaert @ivarconr
Update providers/unleash/README.md
c0ddb56 
Co-authored-by: Ivar Conradi Østhus <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member

toddbaert commented Sep 27, 2023 •
edited
Loading

@liran2000 the release setup looks good. When we merge this and the associated release PR, it will release a 0.0.2 version which is fine with me. We don't publish snapshots (in the java sense) from this repo, but we could (I believe) make the version number something like 0.0.1-alpha or 0.0.1-prerelease if you'd like to have something that seems less "final" for now, which might be a good idea. cc @ivarconr

@liran2000 Thanks for all your efforts on this. It's been requested by the community more than once!

Please see the 2 unresolved issues above.

@liran2000
update owners and test
4a61e9c 
Signed-off-by: liran2000 <[email protected]>
@liran2000
Copy link
Member Author

Thanks :)
addressed latest comments.

@liran2000
update version to alpha
e50eaff 
Signed-off-by: liran2000 <[email protected]>
toddbaert
toddbaert approved these changes Sep 27, 2023
View reviewed changes
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.

I'll merge this before EOD unless I hear objections.

Thanks again all, especially @liran2000

@toddbaert toddbaert merged commit ec6b3c9 into open-feature:main Sep 28, 2023
4 checks passed
@github-actions github-actions bot mentioned this pull request Sep 28, 2023
Merged
@liran2000 liran2000 deleted the feature/unleash branch September 28, 2023 05:43
@liran2000 liran2000 mentioned this pull request Oct 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastonfournier gastonfournier gastonfournier left review comments

@ivarconr ivarconr ivarconr left review comments

@toddbaert toddbaert toddbaert approved these changes

@sighphyre sighphyre Awaiting requested review from sighphyre

Assignees

@liran2000 liran2000

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liran2000 @ivarconr @toddbaert @gastonfournier @sighphyre