-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
client/v3/watch.go: use fmt go pkg for gRPC metadata map printing #18308
Conversation
Hi @mohamedawnallah. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
37580b0
to
9d7597d
Compare
fmt
go pkg for metadata map printing
/ok-to-test |
Please also add an unit test |
9d7597d
to
ed6b76a
Compare
ed6b76a
to
9b541c4
Compare
Signed-off-by: Mohamed Awnallah <[email protected]>
9b541c4
to
7b776e2
Compare
7b776e2
to
559017c
Compare
Thanks a lot for the feedback, @ahrtr! It is much better now! 🙏 I have addressed all of the feedback. |
Signed-off-by: Mohamed Awnallah <[email protected]> Co-authored-by: Benjamin Wang <ahrtr>
559017c
to
1e620ed
Compare
I've addressed the recent feedback 🙏. This might be off-topic, but I'm eager to contribute more to |
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
nice work! Thank you for your first contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Thanks, @mohamedawnallah. I left a potential suggestion.
I also suggest removing the "Close" keyword from the pull request description, as Benjamin labeled the issue with a backport to 3.4 and 3.5, and it would be better not to close the parent issue automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can add completeness in a follow-up/later. Thanks.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, ivanvc, mohamedawnallah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@mohamedawnallah could you backport the PR to 3.5 and 3.4? |
@ahrtr Yeah I would love that it is my first time going through this backport process! I've just submitted two PRs for this:
|
fmt
go pkg for metadata map printing
Description
Addresses this issue #18303