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

protoc-gen-go-grpc: copy service comment to interfaces #7243

Merged
merged 2 commits into from
May 22, 2024

Conversation

silves-xiang
Copy link
Contributor

@silves-xiang silves-xiang commented May 18, 2024

This PR to fix Bug #7233 , copy proto comment to pb.go file
I am very interested in this project. If there are any problems with the code I mentioned, please point it out and I will modify it immediately and submit it again. Thanks!
This PR adds an internal only (exported, but can only set through internal/) LateApplyDialOption type. This will be used in CSM Observability.

RELEASE NOTES: N/A

Copy link

linux-foundation-easycla bot commented May 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: silves-xiang (36e4b25)
  • ✅ login: dfawley / name: Doug Fawley (b6cbade)

@silves-xiang
Copy link
Contributor Author

I think there are two tests failed, should modify test case? I am so sorry, i can't know it, please maintainer to answer me and i modify immediately. Thanks!

@purnesh42H purnesh42H added Type: Documentation Documentation or examples Type: Feature New features or improvements in behavior and removed Type: Documentation Documentation or examples labels May 20, 2024
@silves-xiang silves-xiang removed their assignment May 20, 2024
@silves-xiang
Copy link
Contributor Author

I have modified the comment. I don’t know what I need to do next. If there is anything else I need to do, please explain it to me. Thank you. @purnesh42H

@purnesh42H purnesh42H self-assigned this May 20, 2024
@purnesh42H
Copy link
Contributor

purnesh42H commented May 20, 2024

@silves-xiang could you provide the two tests which are failing? I only see Mergeable check failing

@silves-xiang
Copy link
Contributor Author

@silves-xiang could you provide the two tests which are failing?@silves-fang 你能提供两个失败的测试吗?

The failed test has been fixed. The reason is that the pb.go generated by the proto file of the automated test was not updated in time, causing the automated test to be considered a failure.

@silves-xiang
Copy link
Contributor Author

@purnesh42H At the beginning, I only modified the program code and not the automatically generated pb.go file, which resulted in an automatic failure. I have regenerated the pb.go file generated by proto in time.

@silves-xiang
Copy link
Contributor Author

silves-xiang commented May 20, 2024

@purnesh42H Hello, I just modified it, but I don’t know if it is in line with what you meant. If it is wrong, please correct me. I will modify immediately, Thanks!

@dfawley dfawley changed the title Feat:copy service comment to interfaces protoc-gen-go-grpc: copy service comment to interfaces May 20, 2024
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
health/grpc_health_v1/health_grpc.pb.go Outdated Show resolved Hide resolved
health/grpc_health_v1/health_grpc.pb.go Outdated Show resolved Hide resolved
@silves-xiang silves-xiang force-pushed the master branch 2 times, most recently from 8636196 to 467a091 Compare May 21, 2024 03:43
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.55%. Comparing base (adf976b) to head (467a091).
Report is 59 commits behind head on master.

Current head 467a091 differs from pull request most recent head b6cbade

Please upload reports for the commit b6cbade to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7243      +/-   ##
==========================================
- Coverage   81.24%   80.55%   -0.69%     
==========================================
  Files         345      349       +4     
  Lines       33941    34032      +91     
==========================================
- Hits        27574    27415     -159     
- Misses       5202     5428     +226     
- Partials     1165     1189      +24     

see 35 files with indirect coverage changes

@silves-xiang silves-xiang force-pushed the master branch 2 times, most recently from 72c4cbb to cf5b4f5 Compare May 21, 2024 04:48
@silves-xiang
Copy link
Contributor Author

@purnesh42H @dfawley Already modify file, Please check it.If you still have any questions, please let me know. Thanks

Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

LGTM. Let @dfawley review it once

@purnesh42H purnesh42H assigned dfawley and unassigned dfawley and purnesh42H May 21, 2024
@purnesh42H purnesh42H added the P3 label May 21, 2024
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/grpc.go Outdated Show resolved Hide resolved
@silves-xiang
Copy link
Contributor Author

@purnesh42H @dfawley It has been processed, please check it. If there is any problem, please tell me and I will modify it immediately and resubmit it.Thanks

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.

LGTM, thank you for the contribution

@purnesh42H purnesh42H added this to the 1.65 Release milestone May 22, 2024
@dfawley dfawley merged commit 1adbea2 into grpc:master May 22, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants