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.

Sign up for GitHub

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

Jump to bottom

reflection: expose both v1 and v1alpha reflection services #6329

Merged
merged 3 commits into from Jun 12, 2023
Merged

reflection: expose both v1 and v1alpha reflection services #6329

merged 3 commits into from Jun 12, 2023

Conversation

jhump
Copy link
Member

@jhump jhump commented May 31, 2023 •
edited by dfawley

Resolves #5684.

This adds RegisterV1 and RegisterV1Alpha to provide control over the versions registered. The existing Register function registers both versions.

This also adds NewServerV1 to return a server that implements the new version. NewServer continues to return the v1alpha version of the interface.

All of the core logic and types have been updated to refer to the newer definitions in v1. Adapters have been added, to convert between v1 and v1alpha, to provide the v1alpha server. The adapters are also used in tests, so that the end-to-end tests now go through both v1 and v1alpha stubs, to verify both versions work correctly.

RELEASE NOTES:

  • reflection: support the v1 reflection service and update Register to register both v1alpha and v1.

@jhump
Copy link
Member Author

jhump commented May 31, 2023

@dfawley, sorry for not waiting for an answer to my question on #5684. I guess I was optimistic that maybe you'd accept a pull request for it. Let me know what you think.

@dfawley dfawley self-requested a review May 31, 2023 20:24
@dfawley dfawley self-assigned this May 31, 2023
@dfawley dfawley added this to the 1.57 Release milestone Jun 2, 2023
dfawley
dfawley approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here... This LGTM with a few minor comments. We also will need a second review before we can merge.

reflection/adapt.go Outdated Show resolved Hide resolved
reflection/serverreflection.go Outdated Show resolved Hide resolved
reflection/serverreflection.go Outdated Show resolved Hide resolved
@dfawley dfawley requested a review from zasweq June 9, 2023 23:52
@dfawley dfawley added the Type: Feature New features or improvements in behavior label Jun 9, 2023
@dfawley dfawley changed the title Expose both v1 and v1alpha reflection services reflection: expose both v1 and v1alpha reflection services Jun 9, 2023
zasweq
zasweq approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution.

@zasweq zasweq merged commit 642dd63 into grpc:master Jun 12, 2023
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

@zasweq zasweq zasweq approved these changes

Assignees

@dfawley dfawley

@zasweq zasweq

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.57 Release
Development

Successfully merging this pull request may close these issues.

support "v1" of the reflection protocol, not just "v1alpha"
3 participants
@jhump @dfawley @zasweq