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

Grab comment from proto file, similar to protoc-gen-go #5540

Merged

Conversation

RedHawker
Copy link
Contributor

@RedHawker RedHawker commented Jul 26, 2022

Adding initial code. grabs the header comment from the FileDescriptor
This code was copied from the main protoc-gen-go code

Attempts to address #5530

It's quick and dirty, but I'm going people who know better about the project can suggest a better way.

RELEASE NOTES: n/a

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 26, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 4, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Aug 4, 2022
@RedHawker
Copy link
Contributor Author

The vet-proto step fails in the checks with the proposed changes, what needs to change to allow for them to work with the generated files now containing the additional comment(s)?

@github-actions github-actions bot removed the stale label Aug 11, 2022
@dfawley
Copy link
Member

dfawley commented Aug 11, 2022

The vet-proto step fails in the checks with the proposed changes, what needs to change to allow for them to work with the generated files now containing the additional comment(s)?

Changes to the code generator will require that you re-run ./regenerate.sh from the root of the repo. That will also give us a preview of how the generated code looks.

@dfawley dfawley added the Type: Feature New features or improvements in behavior label Aug 11, 2022
@RedHawker
Copy link
Contributor Author

The vet-proto step fails in the checks with the proposed changes, what needs to change to allow for them to work with the generated files now containing the additional comment(s)?

Changes to the code generator will require that you re-run ./regenerate.sh from the root of the repo. That will also give us a preview of how the generated code looks.

I ran regenerate.sh but it doesn't run in a container, so it uses my system's protoc version instead of 3.14.0 that's in the existing files.

@dfawley
Copy link
Member

dfawley commented Aug 11, 2022

I ran regenerate.sh but it doesn't run in a container, so it uses my system's protoc version instead of 3.14.0 that's in the existing files.

Would you mind downloading that version and making sure it gets run instead?

      PROTOBUF_VERSION=3.14.0
      PROTOC_FILENAME=protoc-${PROTOBUF_VERSION}-linux-x86_64.zip
      pushd /home/travis
      wget https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/${PROTOC_FILENAME}

@dfawley dfawley added this to the 1.50 Release milestone Aug 12, 2022
@RedHawker
Copy link
Contributor Author

I don't know why those tests are failing, they seem unrelated to my changes as far as I can tell.

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

LGTM. Can merge once the nits are taken care of.

cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@RedHawker
Copy link
Contributor Author

I'm not sure what else needs to be done?

@github-actions github-actions bot removed the stale label Sep 6, 2022
@easwars easwars merged commit 182e9df into grpc:master Sep 6, 2022
1 check passed
@easwars
Copy link
Contributor

easwars commented Sep 6, 2022

Thank you for the PR !!

@andremarianiello
Copy link

Now that this is fixed, is there going to be a new version tag for protoc-gen-go-grpc?

@easwars
Copy link
Contributor

easwars commented Feb 28, 2023

Now that this is fixed, is there going to be a new version tag for protoc-gen-go-grpc?

Yes, we will get that out soonish. Filed #6060 to track.

@arvindbr8
Copy link
Member

Hi @andremarianiello, you should find the fix included in our latest release of proto-gen-go-gRPC. Please feel free to reach out if there are any concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Requires Reporter Clarification Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants