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

rls: double import rls protos #5003

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 23, 2021

Use rlspb for messages and rlsgrpc for services

RELEASE NOTES: n/a

Use `rlspb` for messages and `rlsgrpc` for services
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Nov 23, 2021
@easwars easwars added this to the 1.43 release milestone Nov 23, 2021
@dfawley
Copy link
Member

dfawley commented Nov 24, 2021

Will this conflict with your current RLS PRs? If so I don't know if this is worth doing now; let's just do it as part of that work. But if this file isn't changing, then this is fine to do now.

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.

Are there no test changes needed? That would be surprising.

@dfawley dfawley assigned easwars and unassigned dfawley and zasweq Nov 24, 2021
@easwars
Copy link
Contributor Author

easwars commented Nov 29, 2021

Will this conflict with your current RLS PRs? If so I don't know if this is worth doing now; let's just do it as part of that work. But if this file isn't changing, then this is fine to do now.

Actually, I have created a new file control_channel.go which contains the control channel functionality. So, this file will eventually be deleted. But I like having the double import in there as just a visual re-enforcement to remind me to do that everytime :)

@easwars
Copy link
Contributor Author

easwars commented Nov 29, 2021

Are there no test changes needed? That would be surprising.

We currently only have a unit test which tests the rlsClient type. So, the test doesn't currently bring up any RLS service. So, it was not affected.

@dfawley
Copy link
Member

dfawley commented Nov 29, 2021

We currently only have a unit test which tests the rlsClient type. So, the test doesn't currently bring up any RLS service. So, it was not affected.

This is a test coverage gap for now, then.

Actually, I have created a new file control_channel.go which contains the control channel functionality.

Please make sure that one does the double-import too!

@easwars easwars merged commit 6f8796b into grpc:master Nov 29, 2021
@easwars easwars deleted the rls_split_proto_imports branch November 29, 2021 22:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants