Skip to content

feat: ofrep provider #53

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

Merged
merged 7 commits into from
Jul 24, 2025
Merged

Conversation

Rahul-Baradol
Copy link
Member

@Rahul-Baradol Rahul-Baradol commented Jun 24, 2025

This PR

  • adds OFREP provider

Related Issues

Fixes #51

Follow-up Tasks

  • Dev testing
  • Unit tests
  • Docs

Signed-off-by: Rahul Baradol <[email protected]>
@Rahul-Baradol
Copy link
Member Author

@erenatas
PR is ready for review... please let me know your thoughts!

@erenatas
Copy link
Contributor

You are quick :) I will have a look today

@erenatas
Copy link
Contributor

@beeme1mr Hey, do we have any behavioral tests for OFREP?

@Rahul-Baradol it looks like a nice start, however I don't see any have documentation, code coverage. You can have a look at what we have done over flagd

@Rahul-Baradol
Copy link
Member Author

Yeah right...took this as a start, and wanted your input before proceeding.
No problem, will write the tests and docs, and we can take from there!

@erenatas
Copy link
Contributor

Yeah right...took this as a start, and wanted your input before proceeding. No problem, will write the tests and docs, and we can take from there!

Its a good start :). The implementation reuses the same reqwest client as well 👍

@Rahul-Baradol
Copy link
Member Author

@erenatas
have added UTCs and docs...

Signed-off-by: Rahul Baradol <[email protected]>
@Rahul-Baradol Rahul-Baradol requested a review from erka June 29, 2025 14:08
Copy link

@erka erka left a comment

Choose a reason for hiding this comment

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

I’ve made some modifications to your implementation in this PR

@erenatas
Copy link
Contributor

erenatas commented Jul 1, 2025

Having a look at: https://github.com/open-feature/protocol/blob/main/guideline/dynamic-context-provider.md You did most of the work so big kudos!

I see 2 things are missing:

  1. provider can receive 429 response (rate limiting):

    429: The provider has reached the rate limit of the flag management system. In that situation the provider should read the Retry-After header from the response and ensure that no call to the endpoint happened before this date. Every evaluation happening before the retry after date should error and return the default value.
    In this case you should handle this in your responses such as:

    match response.status() {
        // ... existing cases ...
        StatusCode::TOO_MANY_REQUESTS => {
            ....
    }

    user should be caching this on the client side so perhaps you can return an Err

  2. guideline says:

    baseURL: The base URL of the flag management system.
    This must be the base of the URL pointing before the /ofrep namespace of the API.
    In the constructor, the provider should check if the baseURL is a valid URL and return an error if the URL is invalid.

    You can utilize url crate for handling this.

Signed-off-by: Rahul Baradol <[email protected]>
Copy link

@erka erka left a comment

Choose a reason for hiding this comment

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

Great start @Rahul-Baradol ! There might be a few things missing, but it’s definitely headed in the right direction.

Signed-off-by: Rahul Baradol <[email protected]>
@erenatas
Copy link
Contributor

@Rahul-Baradol, hi, I was away for the past 2 weeks but I will be back from next week. Let me know when its ready for review, and I will have a look.

@beeme1mr
Copy link
Member

Hey, do we have any behavioral tests for OFREP?

@Rahul-Baradol @erenatas here's what we currently have for requirements https://github.com/open-feature/protocol/blob/main/guideline/dynamic-context-provider.md. Please note that this is for dynamic context providers (e.g. server).

@erenatas
Copy link
Contributor

Hey, do we have any behavioral tests for OFREP?

@Rahul-Baradol @erenatas here's what we currently have for requirements https://github.com/open-feature/protocol/blob/main/guideline/dynamic-context-provider.md. Please note that this is for dynamic context providers (e.g. server).

Hey, overall this implementation looks good to me as well. Perhaps we can do a 0.0.1 release, wdyt?

@erenatas erenatas self-requested a review July 20, 2025 20:48
@Rahul-Baradol
Copy link
Member Author

Sure @erenatas we can go ahead !

@beeme1mr beeme1mr merged commit d0b9d76 into open-feature:main Jul 24, 2025
4 checks passed
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.

[FEATURE] Create OFREP provider
4 participants