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 ConfigCat provider #119

Merged
merged 23 commits into from
Jan 16, 2024
Merged

feat: Add ConfigCat provider #119

merged 23 commits into from
Jan 16, 2024

Conversation

luizbon
Copy link
Contributor

@luizbon luizbon commented Dec 21, 2023

This PR

  • Adding ConfigCat provider integration

How to test

Have a ConfigCat free account, create Product/Config/Keys and use as any other OpenFeature provider.

@luizbon luizbon requested a review from a team as a code owner December 21, 2023 00:49
@luizbon luizbon changed the title Feat/add configcat provider Feat: Add ConfigCat provider Dec 21, 2023
@luizbon luizbon changed the title Feat: Add ConfigCat provider feat: Add ConfigCat provider Dec 21, 2023
laliconfigcat
laliconfigcat suggested changes Dec 21, 2023
View reviewed changes
Copy link

@laliconfigcat laliconfigcat left a comment

Choose a reason for hiding this comment

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

Hello @luizbon ,

Thank you very much for your contribution! We really appreciate it at ConfigCat.
I left some minor comments, but I think it looks good.

Have a nice day!

src/OpenFeature.Contrib.ConfigCat/ConfigCatProvider.cs Outdated Show resolved Hide resolved
src/OpenFeature.Contrib.ConfigCat/ConfigCatProvider.cs Outdated Show resolved Hide resolved
src/OpenFeature.Contrib.ConfigCat/ConfigCatProvider.cs Outdated Show resolved Hide resolved
src/OpenFeature.Contrib.ConfigCat/UserBuilder.cs Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

I'll give this a review tomorrow! Thanks @luizbon

renovate bot and others added 13 commits December 22, 2023 09:16
@renovate @luizbon
chore(deps): update dependency grpc.net.client to v2.59.0 (open-featu…
2b187ad 
…re#115)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Add ConfigCat provider
2230d67 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Add message to unsupported feature
2150d2d 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Implement Stucture with JSON string
53505d8 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Refactor ConfigCatProvider and UserBuilder classes
69dc9ee 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Add ConfigCat provider
5ce4344 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Add message to unsupported feature
b76a5a8 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Implement Stucture with JSON string
254a50b 
Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Refactor ConfigCatProvider and UserBuilder classes
26d00cc 
Signed-off-by: Luiz Bon <[email protected]>
@renovate @luizbon
chore(deps): update dotnet monorepo to v8 (major) (open-feature#100)
eddd56f 
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Luiz Bon <[email protected]>
@vpetrusevici @luizbon
fix: do not send targeting key as separate trait in flagsmith (open-f…
7b1a329 
…eature#120)

Signed-off-by: Vladimir Petrusevici <[email protected]>
Signed-off-by: Luiz Bon <[email protected]>
@github-actions @luizbon
chore(main): release OpenFeature.Contrib.Providers.Flagsmith 0.1.5 (o…
da99c30 
…pen-feature#121)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@renovate @luizbon
chore(deps): update dependency grpc.tools to v2.60.0 (open-feature#111)
77cf631 
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Luiz Bon <[email protected]>
toddbaert
toddbaert requested changes Dec 29, 2023
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wow @luizbon ! Thanks so much for this contribution, and sorry my review was delayed (holidays 😅 )

I left a few nits which are up to you. I think the only required changes from my perspective are to add a README and this.

I've also opened a PR to this branch to add you and @laliconfigcat to the owners file here.

adams85
adams85 suggested changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@adams85 adams85 left a comment

Choose a reason for hiding this comment

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

Hi, I'm Adam from ConfigCat, I'm the primary maintainer of the ConfigCat .NET SDK currently. This is why @laliconfigcat asked me to take a look at this PR.

Here are my findings.

src/OpenFeature.Contrib.ConfigCat/ConfigCatProvider.cs Outdated Show resolved Hide resolved
src/OpenFeature.Contrib.ConfigCat/ConfigCatProvider.cs Outdated Show resolved Hide resolved
test/OpenFeature.Contrib.ConfigCat.Test/ConfigCatProviderTest.cs Outdated Show resolved Hide resolved
@luizbon
Merge remote-tracking branch 'upstream/main' into feat/add-configcat-…
ce31dda 
…provider

# Conflicts:
#	.release-please-manifest.json

Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Address PR comments
eafc48c 
- Added UserBuilder tests
- Throw `FeatureProviderException` on resolve failures
- Make ConfigCatProvider Disposable
- Added README file

Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Copy link
Contributor Author

luizbon commented Jan 4, 2024

Hi @toddbaert and @adams85, sorry for the delay, I just came back from holidays.
I think I have addressed all your comments. Can you please review it again?

@adams85 adams85 mentioned this pull request Jan 4, 2024
Merged
4 tasks
@beeme1mr
Copy link
Member

@toddbaert could you please review this tomorrow?

@luizbon could you please address the formatting issue when you have a moment?

@luizbon
Copy link
Contributor Author

luizbon commented Jan 10, 2024

Thanks for reminding me @beeme1mr, I have applied the format fix.

toddbaert
toddbaert approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@luizbon Thanks again for your contribution, and your patience with the review process. I am approving, however, I will request one more change (I'd do it myself if I could, but I can't modify your branch): please add yourself (and perhaps @laliconfigcat and @adams85 if they are interested) to the component_owners for this component, and it's tests. This will add you to PRs for this component, and really helps share the maintenance load.

@laliconfigcat if you end up making changes to the config-cat SDK to send some kind of error code as you mention here, please create a PR or issue to improve the handling here if you dont mind! 🙏

@luizbon
Add myself to component owners
1c73522 
Fix project paths and package id

Signed-off-by: Luiz Bon <[email protected]>
@luizbon
Copy link
Contributor Author

luizbon commented Jan 15, 2024

Thanks, @toddbaert; just made the change to the component owner file and also found I needed to fix the path and package ID for the project.
It should be good to go now.

@toddbaert
Copy link
Member

@adams85 @laliconfigcat are either of you interested in being added to the compoent_owners? This will add your automatically to any PRs impacting this component. We only require one, so @luizbon 's addition is sufficient (note you will also have to be an org member for this automation to work).

In any case, I will merge this tomorrow EOD unless I hear objections! Thanks @luizbon !

@laliconfigcat
Copy link

@adams85 @laliconfigcat are either of you interested in being added to the compoent_owners? This will add your automatically to any PRs impacting this component. We only require one, so @luizbon 's addition is sufficient (note you will also have to be an org member for this automation to work).

In any case, I will merge this tomorrow EOD unless I hear objections! Thanks @luizbon !

Thanks for the suggestion, but I just jumped in for a quick PR review. I think @luizbon is the real component owner here :).

@toddbaert toddbaert merged commit 20aeb3a into open-feature:main Jan 16, 2024
9 checks passed
This was referenced Jan 16, 2024
Merged
Closed
@github-actions github-actions bot mentioned this pull request Jun 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddbaert toddbaert toddbaert approved these changes

@adams85 adams85 adams85 approved these changes

@bacherfl bacherfl Awaiting requested review from bacherfl

@matthewelwell matthewelwell Awaiting requested review from matthewelwell

@thomaspoignant thomaspoignant Awaiting requested review from thomaspoignant

@vpetrusevici vpetrusevici Awaiting requested review from vpetrusevici

@laliconfigcat laliconfigcat Awaiting requested review from laliconfigcat

Assignees

@luizbon luizbon

@bacherfl bacherfl

@matthewelwell matthewelwell

@thomaspoignant thomaspoignant

@toddbaert toddbaert

@vpetrusevici vpetrusevici

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@luizbon @toddbaert @beeme1mr @laliconfigcat @adams85 @bacherfl @matthewelwell @thomaspoignant @vpetrusevici