-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Implement new reflection v1 server and public API #5773
Conversation
We want both the
Another thing to take care of, which is not directly related to the changes that are being made in this PR, is to have separate imports for messages and services. See here for an explanation of why we need to have separate imports. |
@buzzsurfr : Is this PR only adding the new files? If that is the case, this PR should not change other documentation and source files. If we agree on doing the actual implementation in a follow up PR, we need to change the PR description and associated release notes entry as well. Also, looks like there is a |
@easwars: I fixed the My intent was to change the documentation locally in the repo to reflect v1 and v1alpha being available. Let's implement v1 as the standard, or build an abstraction layer (interface I'm guessing) so that we can change the default from v1alpha to v1 at a later date. This way it wouldn't affect current builds but would maintain forward compatibility. Given that it's my first PR on this org, can you provide additional guidance/suggestions? 😄 /remove label:"Status: Requires Reporter Clarification" |
48967ff
to
9eb47fa
Compare
The
The Changes that need to be made for us to support
Since we support Go versions older than 1.18, the implementation will not be able to use generics and hence there will be some amount of code duplication, and that is fine. Let me know what you think of this approach. |
That makes sense to me. I'll split this into 3 PRs. For the last one, I was also considering an interface where we could create a new server that takes the API version as an argument (default to v1alpha for now). Then we can also flip it at a later date. |
Thanks @buzzsurfr. We can probably repurpose this PR for first item in the list. Do let me know when you have updated it and I will take another look. |
Created #5799 as the first step to pull in protobufs from the remote repo and regenerate. Working on the next two now, but going to close this one and move to the new PR (for a clean history). |
ce48e22
to
3de90d8
Compare
@easwars Rebased this branch and reused the PR for the second and third items. Description has been changed. I'm leaving this on draft as the changes is #5799 need to be merged before this PR. This also does NOT include adding v1 to the default implementation--but this gets the codebase ready to do so. |
CHANGES:
rpb
torv1alphapb
in reflection implementation. This allows for adding support for v1 into the registration in the future.serverReflectionServerV1
which implements the v1 serviceNewServerV1
andRegisterV1
as duplicates that use the v1 server (was necessary for proper testing).Works on #5684 but will require a future commit that implements both v1alpha and v1.
Depends on merging #5799 (tests will fail until merge and this will be rebased)
RELEASE NOTES:
rpb
torv1alphapb
in reflection implementationserverReflectionServerV1
and new functionsNewServerV1
andRegisterV1
which use reflection v1