Skip to content

feat(flagsmith-provider): Add flagsmith provider #128

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
Merged
merged 23 commits into from
Mar 15, 2023
Merged

feat(flagsmith-provider): Add flagsmith provider #128

merged 23 commits into from
Mar 15, 2023

Conversation

gagantrivedi
Copy link
Contributor

@gagantrivedi gagantrivedi commented Feb 28, 2023 •
edited
Loading

This PR

Related Issues

N/A

Notes

N/A

Follow-up Tasks

N/A(?)

How to test

Includes unit tests

@gagantrivedi gagantrivedi force-pushed the feat/flagsmith-provider branch from aa86d11 to d1bcfe6 Compare February 28, 2023 04:31
@beeme1mr
Copy link
Member

Hey @gagantrivedi, thanks for your contribution. Please let me know when you're ready for a review.

@gagantrivedi gagantrivedi force-pushed the feat/flagsmith-provider branch 2 times, most recently from e08bce2 to b2a8fdd Compare March 1, 2023 15:46
@gagantrivedi
init
f1f3db3 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
implement method to evaluate flags generically
f982c19 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>

Implement evaluation methods

Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>

misc fixes

Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
create WithUsingBooleanConfigValue option
c9d19da 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Add test for int evaluation
3a5c425 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Add tests for float evaluation
78853da 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Add tests for string evaluation
6aa2033 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Add readme
95bcedc 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
reformat
a4f71b9 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
misc: better errors/docs
ed1f33e 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
tests: Add Boolean evaluation tests
cad85b0 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Add tests for Object evaluation
b9f9d1e 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
lint: remove lint errors
5071c96 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
fix: disabled flags using default value
52723a8 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
fix usingBooleanConfigValue
8e9ad85 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
update go.mod
3a64371 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
update readme
b2a8fdd 
minor style changes

Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi gagantrivedi changed the title WIP:feat(flagsmith-provider): Add flagsmith provider feat(flagsmith-provider): Add flagsmith provider Mar 1, 2023
@gagantrivedi gagantrivedi marked this pull request as ready for review March 1, 2023 15:54
@gagantrivedi
Copy link
Contributor Author

gagantrivedi commented Mar 1, 2023 •
edited
Loading

Hey @gagantrivedi, thanks for your contribution. Please let me know when you're ready for a review.

Hi @beeme1mr it's ready for review now.
PS: Please feel free to nitpick

beeme1mr
beeme1mr reviewed Mar 1, 2023
View reviewed changes
gagantrivedi and others added 2 commits March 6, 2023 21:54
@gagantrivedi @beeme1mr
Update providers/flagsmith/README.md fix typo
dc1efe5 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Gagan <gagandeeptrivedi47@gmail.com>
@gagantrivedi @beeme1mr
Update providers/flagsmith/README.md fix typo
86c9a33 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Gagan <gagandeeptrivedi47@gmail.com>
@beeme1mr beeme1mr requested a review from james-milligan March 7, 2023 20:23
@Kavindu-Dodan
Copy link
Contributor

@gagantrivedi Thank you for the contribution. I think you need to add a new entry to .release-please-manifest.json [1]. This controls the release please based versioning.

[1] - https://github.com/open-feature/go-sdk-contrib/blob/main/.release-please-manifest.json

Kavindu-Dodan
Kavindu-Dodan reviewed Mar 7, 2023
View reviewed changes
Kavindu-Dodan
Kavindu-Dodan reviewed Mar 7, 2023
View reviewed changes
Kavindu-Dodan
Kavindu-Dodan reviewed Mar 7, 2023
View reviewed changes
Kavindu-Dodan
Kavindu-Dodan reviewed Mar 7, 2023
View reviewed changes
Kavindu-Dodan
Kavindu-Dodan reviewed Mar 7, 2023
View reviewed changes
@Kavindu-Dodan
Copy link
Contributor

Other than the my comments, looks good in my view 👍

@toddbaert
Copy link
Member

@gagantrivedi you may want to add this to https://github.com/open-feature/go-sdk-contrib/blob/main/.release-please-manifest.json with an explicit sub 1.0 version. Otherwise it will be released as 1.0 (unless you want that).

toddbaert
toddbaert approved these changes Mar 8, 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.

Looks great to me from a provider/sdk/spec perspective. I think I agree with all of @Kavindu-Dodan 's points though. Also please note this.

@toddbaert
Copy link
Member

@gagantrivedi Thank you for the contribution. I think you need to add a new entry to .release-please-manifest.json [1]. This controls the release please based versioning.

[1] - https://github.com/open-feature/go-sdk-contrib/blob/main/.release-please-manifest.json

It's not STRICTLY necessary, but if it's not added here it will be a 1.0.

matthewelwell
matthewelwell reviewed Mar 9, 2023
View reviewed changes
Copy link
Member

@matthewelwell matthewelwell 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. Added one comment regarding consistency with other Flagsmith providers.

providers/flagsmith/README.md Outdated
....

// With traits
trait := flagsmith.Trait{TraitKey: "of_key", TraitValue: "of_value"}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can prevent the user from having to use Flagsmith internal structs here? For example, in the Java provider, we just use ctx.AsObjectMap() to provide the traits. See here:

https://github.com/open-feature/java-sdk-contrib/blob/main/providers/flagsmith/src/main/java/dev.openfeature.contrib.providers.flagsmith/FlagsmithProvider.java#L106

I appreciate maybe the Go client requires Trait structs but we could do that under the hood in the provider somewhere?

Copy link
Contributor Author

@gagantrivedi gagantrivedi Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, good point. It's done

@gagantrivedi
fix: Enable support for go 1.18
11bf262 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
misc fixes
3fe4869 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
Merge branch 'main' into feat/flagsmith-provider
ffea073 
Signed-off-by: Gagan <gagandeeptrivedi47@gmail.com>
@gagantrivedi
remove newline
950fffe 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi
fix(traits): use evalCtx as traits
4bdba82 
Signed-off-by: Gagan Trivedi <gagandeeptrivedi47@gmail.com>
@gagantrivedi gagantrivedi requested review from Kavindu-Dodan and matthewelwell and removed request for skyerus, james-milligan and Kavindu-Dodan March 13, 2023 09:01
@beeme1mr beeme1mr self-requested a review March 15, 2023 13:43
@beeme1mr
Copy link
Member

@matthewelwell, do you have any more concerns? If not, I'll merge this today and cut a release. Thanks for putting this together, @gagantrivedi!

@matthewelwell
Copy link
Member

@matthewelwell, do you have any more concerns? If not, I'll merge this today and cut a release. Thanks for putting this together, @gagantrivedi!

No more concerns from me, looks good! Thanks @beeme1mr @gagantrivedi .

@beeme1mr beeme1mr merged commit 185b721 into open-feature:main Mar 15, 2023
@github-actions github-actions bot mentioned this pull request Mar 15, 2023
Merged
@rcrowe rcrowe mentioned this pull request Jun 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan left review comments

@beeme1mr beeme1mr beeme1mr approved these changes

@toddbaert toddbaert toddbaert approved these changes

@matthewelwell matthewelwell Awaiting requested review from matthewelwell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gagantrivedi @beeme1mr @Kavindu-Dodan @toddbaert @matthewelwell