-
Notifications
You must be signed in to change notification settings - Fork 58
feat(flagd): Implement header-based selector for in-process sync stream connection #1622 #1623
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
|
@copilot please continue |
Co-authored-by: aepfli <[email protected]>
I've documented the selector migration guidance for in-process mode. The changes include:
The documentation emphasizes this only affects in-process resolver mode and provides a clear path forward for users. See commit 0e8b0e6. |
Co-authored-by: aepfli <[email protected]>
|
@copilot i think there was a mixup with the issue titles, can you adopt the grpc connection for the sync stream endpoint to connect via a header for the selector. and not just document it |
Co-authored-by: aepfli <[email protected]>
Co-authored-by: aepfli <[email protected]>
I've implemented the header-based selector approach for the sync stream gRPC connection. The SDK now automatically passes the selector via the Changes made:
All existing tests pass without modification. |
|
@copilot can you fix the java doc the build is failing due to and can you incoporate the feedback from gemini |
Co-authored-by: aepfli <[email protected]>
Fixed the JavaDoc formatting errors in commit d9f840f. All checkstyle violations are now resolved (0 violations). The fixes included:
Build and tests pass successfully. Please provide the specific Gemini feedback you'd like me to incorporate, as I don't have direct access to the review comments. |
Signed-off-by: Simon Schrottner <[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 introduces a header-based selector for the in-process flagd provider, aligning with flagd's selector normalization. The implementation correctly uses a ClientInterceptor to add the flagd-selector header while maintaining backward compatibility by also sending the selector in the request body. The documentation updates in the README and Javadocs are clear and comprehensive. The code is well-structured. I have one suggestion regarding the testability of the new interceptor logic.
| // Apply selector header interceptor if selector is configured | ||
| Channel channel = channelConnector.getChannel(); | ||
| if (selector != null) { | ||
| channel = io.grpc.ClientInterceptors.intercept(channel, createSelectorInterceptor(selector)); | ||
| } |
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.
This new logic for adding the selector interceptor is not covered by existing tests, as they mock the gRPC stubs directly. To improve testability, consider extracting the channel and stub creation logic into a separate, package-private method. This would allow you to write a unit test that verifies the interceptor is correctly added when a selector is present, without needing a full gRPC channel.
|
I am not sure how to really test this properly - we can test if the interceptor is added, but i do not find an easy way to test if the interceptor actually works. - this would be an e2e test scenario, but we would need to somehow add a flag to flagd, to only accept headers, which is imho not worth the effort - please if somebody has an idea, let me know. |
Summary
This PR implements the header-based selector approach for the flagd provider's in-process mode, addressing flagd issue #1814.
Implementation Changes:
SyncStreamQueueSource.java: AddedClientInterceptorthat automatically injects theflagd-selectorheader when selector is configuredDocumentation Updates:
README.md: Updated to reflect automatic header-based implementation, no migration needed for usersFlagdOptions.java: Updated JavaDoc with proper formatting (checkstyle compliant)Key Benefits:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.