-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Revamp protobufs and implement Buf for gRPC stubs #252
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
Conversation
Signed-off-by: kushthedude <kushthedude@gmail.com>
|
@shravanshetty1 The generated files are failing because the stubs need to be regenerated which I am planning to generate with buf in the successive PR which will update the readme too. |
I think its best you do those changes in this PR itself |
shravanshetty1
left a comment
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.
^comment
|
Got it, making the changes now!
…On Sun, 18 Jul, 2021, 23:29 Shravan Shetty, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
^comment
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#252 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLUBSXK74W4SUXZZGRTTYMI7ZANCNFSM5AQ6F5VQ>
.
|
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
|
@shravanshetty1 This is done! All the references & tests have been revamped. |
|
The generated files action I will change when introducing the buf in CI which should go in another PR as this PR has already become quite huge! |
Ideally would have preferred if all buf related changes were in 1 PR including this #250. Its fine if the PR is getting bigger since the tests are ensuring nothing is going to break. |
|
Hey @shravanshetty1 is this good to be merged? |
the ci is still failing, fix the ci |
|
|
|
Let me then remove the generated-file-integrity, because buf has breaking change detection too. Will ping you after updating |
Just use buf in the ci instead of what was there before.... |
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
Signed-off-by: kushthedude <kushthedude@gmail.com>
|
@shravanshetty1 Please review! |
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.
@kushthedude It would be appreciated if you could update naming with go-lint rule and add comment as much as possible. thanks
|
@kushthedude LGTM for now. thanks |
moldis
left a comment
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.
Looks good
fixes #247
fixes #181
fixes #43
Signed-off-by: kushthedude kushthedude@gmail.com
Changes Included:
.which signifies directory tree