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: implement domain scoping #934

Merged

Conversation

jarebudev
Copy link
Contributor

This PR

Related Issues

Resolves #843

Notes

As this change was getting a bit large I wanted to make sure that the main requirements of the issue were implemented correctly first of all so I've not yet done the two requirements that are listed as experimental.

I can either add them into this PR or split them out into a separate issue, either way is fine with me, but wanted to check that this change is on the right path first of all.

Follow-up Tasks

I've not implemented these two requirements

  • Requirement 3.2.2.3: The API MUST have a method for setting evaluation context for a domain.
  • Requirement 3.2.2.4: The API MUST have a mechanism to manage evaluation context for an associated domain.

Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev jarebudev requested a review from a team as a code owner May 13, 2024 22:10
@jarebudev jarebudev changed the title Feat/843 implement domain scoping feat: implement domain scoping May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.27%. Comparing base (4ea74d8) to head (034b066).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #934      +/-   ##
============================================
+ Coverage     95.03%   95.27%   +0.23%     
- Complexity      391      393       +2     
============================================
  Files            37       38       +1     
  Lines           887      888       +1     
  Branches         54       54              
============================================
+ Hits            843      846       +3     
+ Misses           24       23       -1     
+ Partials         20       19       -1     
Flag Coverage Δ
unittests 95.27% <100.00%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@toddbaert
Copy link
Member

I've not implemented these two requirements

Requirement 3.2.2.3: The API MUST have a method for setting evaluation context for a domain.
Requirement 3.2.2.4: The API MUST have a mechanism to manage evaluation context for an associated domain.

These requirements only apply to the static context SDKs (read: client-side SDKs) which Java is not, so you can skip them!

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.

Thanks @jarebudev ! This looks great, there's only a few minor changes that need to be made non-breaking, then I can approve.

Thanks again for your time and effort!

@Kavindu-Dodan
Copy link
Contributor

Thanks @jarebudev ! This looks great, there's only a few minor changes that need to be made non-breaking, then I can approve.

Thanks again for your time and effort!

Same opinion, we can make the change non-breaking. I proposed this

@jarebudev
Copy link
Contributor Author

Thanks @jarebudev ! This looks great, there's only a few minor changes that need to be made non-breaking, then I can approve.
Thanks again for your time and effort!

Same opinion, we can make the change non-breaking. I proposed this

Thanks both for your feedback - I'll apply your suggestions :)

Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented May 24, 2024

@jarebudev thank you for the latest change. I have this doubt and I will approve once I clarify this with @toddbaert :)

I am happy with the rest

@Kavindu-Dodan Kavindu-Dodan added this to the 1.8.1 milestone May 24, 2024
@toddbaert toddbaert self-requested a review May 30, 2024 18:35
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.

Approved. Agree with your deprecation suggestion @Kavindu-Dodan

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor

Approved. Agree with your deprecation suggestion @Kavindu-Dodan

Unfortunately this was not straightforward. We used same Metadata interface for both clients and providers. Given that domains are part of Client, I had to introduce a new interface ClientMetadata. See this commit 2d2c5cf

@toddbaert
Copy link
Member

Approved. Agree with your deprecation suggestion @Kavindu-Dodan

Unfortunately this was not straightforward. We used same Metadata interface for both clients and providers. Given that domains are part of Client, I had to introduce a new interface ClientMetadata. See this commit 2d2c5cf

Isn't this change breaking since we changed a return type?

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented May 31, 2024

Approved. Agree with your deprecation suggestion @Kavindu-Dodan

Unfortunately this was not straightforward. We used same Metadata interface for both clients and providers. Given that domains are part of Client, I had to introduce a new interface ClientMetadata. See this commit 2d2c5cf

Isn't this change breaking since we changed a return type?

Yes, could consider as a breaking change as Client now use ClientMetadata. But if someone use lambda, then it's a simple functional interface with a string return, so shouldn't notice it.

…ossible

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor

Approved. Agree with your deprecation suggestion @Kavindu-Dodan

Unfortunately this was not straightforward. We used same Metadata interface for both clients and providers. Given that domains are part of Client, I had to introduce a new interface ClientMetadata. See this commit 2d2c5cf

Isn't this change breaking since we changed a return type?

Yes, could consider as a breaking change as Client now use ClientMetadata. But if someone use lambda, then it's a simple functional interface with a string return, so shouldn't notice it.

I provided more update here with 12f7f4d

ClientMetadata expose a deprecated getName() so that wherever users have client.getMetadata().getName() is no longer broken. Internally, the returning string matches the domain.

Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Kavindu-Dodan Kavindu-Dodan merged commit 5c0aaaa into open-feature:main Jun 11, 2024
8 checks passed
@jarebudev jarebudev deleted the feat/843_implement_domain_scoping branch June 11, 2024 21:26
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] Implement domain scoping
4 participants