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

health: split imports into healthpb and healthgrpc #5466

Merged
merged 3 commits into from
Jun 24, 2022
Merged

health: split imports into healthpb and healthgrpc #5466

merged 3 commits into from
Jun 24, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Jun 24, 2022

Splitting the imports into pb and grpc is necessary to stop importing these generated files into google3.

RELEASE NOTES: none

@easwars easwars requested a review from zasweq June 24, 2022 00:51
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jun 24, 2022
@easwars easwars added this to the 1.48 Release milestone Jun 24, 2022
zasweq
zasweq requested changes Jun 24, 2022
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.

Your imports aren't alphabetical :). Can you explain why this is needed? Otherwise this LGTM.

@zasweq zasweq assigned easwars and unassigned zasweq Jun 24, 2022
@easwars
Copy link
Contributor Author

easwars commented Jun 24, 2022 •
edited
Loading

Your imports aren't alphabetical :). Can you explain why this is needed? Otherwise this LGTM.

In google3, there are two libraries for every proto. One usually named foo_go_proto which contains the message definitions and another foo_go_grpc which contains service definitions. Unless our imports are split up this way, we will not be able to rename our imports appropriately from our copbara transformation.

As far as the imports not being sorted alphabetically, i'm not sure why gofmt doesn't do that for renamed imports. I did it manually this time around.

@easwars easwars assigned zasweq and unassigned easwars Jun 24, 2022
zasweq
zasweq approved these changes Jun 24, 2022
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!

@zasweq zasweq assigned easwars and unassigned zasweq Jun 24, 2022
@easwars easwars merged commit 15739b5 into grpc:master Jun 24, 2022
@easwars easwars deleted the healthz_pb_gos branch June 24, 2022 20:31
@holdno holdno mentioned this pull request Nov 2, 2022
Closed
@easwars easwars mentioned this pull request Nov 8, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zasweq zasweq zasweq approved these changes

Assignees

@easwars easwars

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.48 Release
Development

Successfully merging this pull request may close these issues.
2 participants
@easwars @zasweq