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: support new flagd.evaluation and flagd.sync schemas #1083

Merged
merged 11 commits into from
Dec 21, 2023

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Dec 15, 2023

Closes #1029

This PR introduces support for the newly introduced evaluation and sync schemas.

Supporting both the old and new schemas involves some duplication, but I tried to keep it as minimal as possible. I'm of course open for suggestions for any ideas on how to make this simpler :)

See reasoning for new naming here.

Copy link

netlify bot commented Dec 15, 2023

Deploy Preview for polite-licorice-3db33c ready!

Name Link
🔨 Latest commit 8102fcf
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/658444912fbb3f0008483478
😎 Deploy Preview https://deploy-preview-1083--polite-licorice-3db33c.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bacherfl bacherfl changed the title Feat/support new schemas feat: support new flagd.evaluation and flagd.sync schemas Dec 15, 2023
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (dea8c78) 72.93% compared to head (8102fcf) 73.53%.
Report is 1 commits behind head on main.

Files Patch % Lines
...e/pkg/service/flag-evaluation/flag_evaluator_v2.go 75.91% 45 Missing and 1 partial ⚠️
...kg/service/flag-evaluation/flag_evaluator_types.go 90.90% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1083      +/-   ##
==========================================
+ Coverage   72.93%   73.53%   +0.59%     
==========================================
  Files          31       32       +1     
  Lines        2864     3113     +249     
==========================================
+ Hits         2089     2289     +200     
- Misses        670      717      +47     
- Partials      105      107       +2     

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

Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
@bacherfl bacherfl marked this pull request as ready for review December 18, 2023 10:20
@bacherfl bacherfl requested a review from a team as a code owner December 18, 2023 10:20
@toddbaert
Copy link
Member

@bacherfl since you're going through this, would you mind sprinkling references to this new issue I created where relevant? That way it'll be super easy to make sure that we remove all the right stuff by just searching for 1088.

@toddbaert
Copy link
Member

Closes #1029

This PR introduces support for the newly introduced evaluation and sync schemas.

Supporting both the old and new schemas involves some duplication, but I tried to keep it as minimal as possible. I'm of course open for suggestions for any ideas on how to make this simpler :)

I think the effort we should put into reducing duplication that we know will be deleted should be low, so unless the solutions are very easy, I'm fine with it as is.

@toddbaert
Copy link
Member

toddbaert commented Dec 18, 2023

Can you update all e2e tests and documentation to use the new paths? I think a good approach might be to simply search for schema.v1.Service and sync.v1 and replace them with the new paths and RPC names. This way new and old users will be more likely to use the non-deprecated stuff.

@toddbaert toddbaert self-requested a review December 18, 2023 18:45
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.

Manually tested and this works for me.

I have 2 small concerns: this and this.

This is optional, but would be nice to make the subsequent removal easier later.

We also could consider adding a middleware (or something like that) that would log a one-time warning when the old endpoint is used.

@bacherfl
Copy link
Contributor Author

Manually tested and this works for me.

I have 2 small concerns: this and this.

This is optional, but would be nice to make the subsequent removal easier later.

We also could consider adding a middleware (or something like that) that would log a one-time warning when the old endpoint is used.

thanks for the review, I will adapt the PR accordingly

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! Thanks a lot!

@toddbaert toddbaert merged commit e9728aa into open-feature:main Dec 21, 2023
15 checks passed
@github-actions github-actions bot mentioned this pull request Dec 21, 2023
toddbaert pushed a commit that referenced this pull request Dec 22, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.8.0</summary>

##
[0.8.0](flagd/v0.7.2...flagd/v0.8.0)
(2023-12-22)


### ⚠ BREAKING CHANGES

* remove deprecated flags
([#1075](#1075))

### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.7.2
([#1056](#1056))
([81e83ea](81e83ea))
* **deps:** update module github.com/spf13/viper to v1.18.0
([#1060](#1060))
([9dfa689](9dfa689))


### 🧹 Chore

* refactoring component structure
([#1044](#1044))
([0c7f78a](0c7f78a))
* remove deprecated flags
([#1075](#1075))
([49f6fe5](49f6fe5))
</details>

<details><summary>flagd-proxy: 0.4.0</summary>

##
[0.4.0](flagd-proxy/v0.3.2...flagd-proxy/v0.4.0)
(2023-12-22)


### ⚠ BREAKING CHANGES

* remove deprecated flags
([#1075](#1075))

### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.7.2
([#1056](#1056))
([81e83ea](81e83ea))
* **deps:** update module github.com/spf13/viper to v1.18.0
([#1060](#1060))
([9dfa689](9dfa689))


### 🧹 Chore

* refactoring component structure
([#1044](#1044))
([0c7f78a](0c7f78a))
* remove deprecated flags
([#1075](#1075))
([49f6fe5](49f6fe5))
</details>

<details><summary>core: 0.7.3</summary>

##
[0.7.3](core/v0.7.2...core/v0.7.3)
(2023-12-22)


### 🐛 Bug Fixes

* **deps:** update golang.org/x/exp digest to 6522937
([#1032](#1032))
([78b23d2](78b23d2))
* **deps:** update module connectrpc.com/connect to v1.13.0
([#1070](#1070))
([63f86ea](63f86ea))
* **deps:** update module github.com/diegoholiveira/jsonlogic/v3 to
v3.4.0 ([#1068](#1068))
([5c5d5ab](5c5d5ab))
* **deps:** update module github.com/open-feature/open-feature-operator
to v0.5.2 ([#1059](#1059))
([cefea3e](cefea3e))
* **deps:** update module google.golang.org/grpc to v1.60.0
([#1074](#1074))
([bf3e9d8](bf3e9d8))
* **deps:** update module google.golang.org/grpc to v1.60.1
([#1092](#1092))
([5bf1368](5bf1368))
* make sure sync builder is initialized to avoid nil pointer access
([#1076](#1076))
([ebcd616](ebcd616))


### ✨ New Features

* support new flagd.evaluation and flagd.sync schemas
([#1083](#1083))
([e9728aa](e9728aa))


### 🧹 Chore

* refactoring component structure
([#1044](#1044))
([0c7f78a](0c7f78a))
* renaming of evaluation components
([#1064](#1064))
([d39f31d](d39f31d))
* use client-go library for retrieving FeatureFlag CRs
([#1077](#1077))
([c86dff0](c86dff0))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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] serve both old and new protos
4 participants