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(flagd-rpc)!: add events for rpc mode, some breaking config fixes #108

Merged
merged 10 commits into from
Nov 28, 2024

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Nov 21, 2024

Adding Events to the RPC client:

Breaking Change

  • renamed resolver type to rpc instead of grpc to be compliant with flagd spec
  • changed configuration values to milliseconds instead of seconds, to be compliant with flagd spec
  • renamed timeout config value to deadline, to be compliant with flagd spec
  • removed property offline source path poll interval as the general approach in other providers is to use retry_backoff_ms

closes: #56

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.92%. Comparing base (61e42e7) to head (68501d0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...enfeature/contrib/provider/flagd/resolvers/grpc.py 94.02% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   93.47%   93.92%   +0.45%     
==========================================
  Files          14       14              
  Lines         582      675      +93     
==========================================
+ Hits          544      634      +90     
- Misses         38       41       +3     

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

@aepfli
Copy link
Member Author

aepfli commented Nov 21, 2024

@toddbaert @beeme1mr @open-feature/sdk-python-maintainers please have a look - this is not perfect - and i might made some errors, but i hope this is somewhat going into the right direction

@aepfli aepfli force-pushed the feat/events-for-rpc branch 2 times, most recently from 04a0ff8 to 572dc51 Compare November 21, 2024 20:20
@aepfli aepfli marked this pull request as ready for review November 22, 2024 09:03
@aepfli aepfli requested a review from a team as a code owner November 22, 2024 09:03
@aepfli aepfli changed the title fix(flagd): adding events for rpc mode feat(flagd): adding events for rpc mode Nov 22, 2024
@aepfli aepfli force-pushed the feat/events-for-rpc branch 2 times, most recently from 57b626b to 5055533 Compare November 22, 2024 10:16
@aepfli
Copy link
Member Author

aepfli commented Nov 22, 2024

Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli closed this Nov 25, 2024
@aepfli aepfli reopened this Nov 26, 2024
@aepfli aepfli marked this pull request as draft November 26, 2024 13:58
@aepfli aepfli force-pushed the feat/events-for-rpc branch 4 times, most recently from 2f89c0b to 4b74a53 Compare November 26, 2024 20:21
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the feat/events-for-rpc branch 2 times, most recently from 5f62c30 to 8b3990f Compare November 26, 2024 20:34
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the feat/events-for-rpc branch from 8b3990f to 9a107c5 Compare November 26, 2024 20:40
@aepfli aepfli marked this pull request as ready for review November 27, 2024 14:53
@aepfli aepfli changed the title feat(flagd): adding events for rpc mode feat(flagd)!: adding events for rpc mode Nov 27, 2024
@aepfli aepfli changed the title feat(flagd)!: adding events for rpc mode feat(flagd-rpc)!: adding events for rpc mode Nov 27, 2024
@toddbaert toddbaert changed the title feat(flagd-rpc)!: adding events for rpc mode feat(flagd-rpc)!: add events for rpc mode, some breaking config fixes Nov 27, 2024
@toddbaert toddbaert self-requested a review November 27, 2024 16:07
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.

I can see the e2e running without the event scenario being skipped now! Very nice to have this feature working and the e2e tests for confidence.

I also went through all the breaking config changes and to me they seem in line with the spec (at least those that we've fully implemented).

I've changed the PR title slightly to (IMO) make the release notes a bit clearer.

…rovider/flagd/resolvers/grpc.py

Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the feat/events-for-rpc branch from b82b626 to 8a794fb Compare November 27, 2024 16:24
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Thanks for adding backwards compatiblity with timeout. The other breaking changes should have virtually no impact because the currently released in-process mode only supported local file watching.

Awesome job! 🥳

Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice work, just one adjustment would be great.

Comment on lines +70 to +71
int(
env_or_default(
Copy link
Member

Choose a reason for hiding this comment

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

nothing you have to fix, but env_or_default() should return the correct type, when cast is set. will try to fix it in a separate PR.

…rovider/flagd/provider.py

Co-authored-by: Anton Grübel <anton.gruebel@gmail.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the feat/events-for-rpc branch from 192f501 to cdd9aa9 Compare November 28, 2024 07:31
Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

@beeme1mr feel free to merge when you are ok with your comments.

@toddbaert toddbaert merged commit b62d3d1 into open-feature:main Nov 28, 2024
25 checks passed
@toddbaert toddbaert deleted the feat/events-for-rpc branch November 28, 2024 15:24
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.

[flagd-RPC] Implement events
4 participants