-
Notifications
You must be signed in to change notification settings - Fork 59
feat(flagd): Add flagd-selector gRPC metadata header support for in-process sync #1399
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
base: main
Are you sure you want to change the base?
Conversation
- Import Metadata from @grpc/grpc-js - Create and populate _metadata with flagd-selector header when selector is configured - Pass metadata to syncFlags gRPC call - Maintain backward compatibility by keeping selector in request field - Add comprehensive tests for selector metadata handling - Document selector implementation and deprecation in README Co-authored-by: aepfli <[email protected]>
Co-authored-by: aepfli <[email protected]>
|
/Gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly implements support for the flagd-selector gRPC metadata header for in-process sync, while maintaining backward compatibility. The implementation is clean, the documentation updates are clear, and the new tests effectively validate the functionality. My review includes suggestions to refactor the new tests to use async/await, which will improve their readability and maintainability. Overall, this is a solid contribution.
libs/providers/flagd/src/lib/service/in-process/grpc/grpc-fetch.spec.ts
Outdated
Show resolved
Hide resolved
libs/providers/flagd/src/lib/service/in-process/grpc/grpc-fetch.spec.ts
Outdated
Show resolved
Hide resolved
@copilot can you improve the tests please based on the example above, using async await |
- Remove done callback pattern from selector tests - Use async/await for cleaner async test handling - Simplifies error handling and improves readability - Follows Jest best practices for async tests Co-authored-by: aepfli <[email protected]>
|
Refactored both tests to use async/await pattern in commit 323bb33. The tests are now cleaner and more readable without the try/catch blocks and manual done() calls. |
Plan: Update selector handling for consistency with sync service (in-process mode only)
Context
Following open-feature/flagd#1814, normalizing selector usage for the sync service (in-process resolver mode) to use
flagd-selectorgRPC metadata header instead of request field.Important: This change only affects in-process resolver mode, not RPC mode.
Changes Implemented
flagd-selectorgRPC metadata headerTechnical Implementation
Modified files:
libs/providers/flagd/src/lib/service/in-process/grpc/grpc-fetch.ts:Metadataimport from@grpc/grpc-js_metadatafield to store gRPC metadataflagd-selectorheader in constructor when selector is configuredsyncFlags()gRPC calllibs/providers/flagd/src/lib/service/in-process/grpc/grpc-fetch.spec.ts:libs/providers/flagd/README.md:Backward Compatibility
The implementation sends the selector in both places during the deprecation period:
flagd-selectorgRPC metadata header (will be the only method in future)selectorfield inSyncFlagsRequest(deprecated, maintained for compatibility)This ensures compatibility with:
Next Steps (future work, not in this PR)
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.