Skip to content

Use Buf linters and formatter on lib/teleterm protos#15877

Merged
codingllama merged 5 commits into
masterfrom
codingllama/protos-teleterm
Aug 29, 2022
Merged

Use Buf linters and formatter on lib/teleterm protos#15877
codingllama merged 5 commits into
masterfrom
codingllama/protos-teleterm

Conversation

@codingllama
Copy link
Copy Markdown
Contributor

@codingllama codingllama commented Aug 26, 2022

Similarly to #15856, moves lib/teleterm fully to Buf.

#15187

@codingllama codingllama requested a review from ravicious August 26, 2022 20:14
@codingllama codingllama force-pushed the codingllama/protos-teleterm branch from 2d25746 to 38fc124 Compare August 26, 2022 20:15
@codingllama codingllama mentioned this pull request Aug 26, 2022
Merged
@codingllama codingllama force-pushed the codingllama/protos-teleterm branch 2 times, most recently from 26477c5 to 29d1520 Compare August 29, 2022 13:12
@codingllama
Copy link
Copy Markdown
Contributor Author

codingllama commented Aug 29, 2022

Friendly ping @ravicious @AntonAM @probakowski ?

@ravicious ravicious mentioned this pull request Aug 29, 2022
Merged
ravicious
ravicious approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

make grpc-teleterm still works as expected. 👍

Comment thread lib/teleterm/api/proto/buf.yaml Outdated
Copy link
Copy Markdown
Member

@ravicious ravicious Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm curious about why this linter is not included with the other linters to fix below.

Tbh, in Connect it kind of doesn't matter because client and server protos are always in sync but it'd probably be good to follow best practices anyway.

Copy link
Copy Markdown
Contributor Author

@codingllama codingllama Aug 29, 2022

Choose a reason for hiding this comment

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

RPC_RESPONSE_STANDARD_NAME and RPC_REQUEST_RESPONSE_UNIQUE make it impossible to follow https://cloud.google.com/apis/design/standard_methods, which I find to be a good standard to follow.

Buf's defaults are OK and we'd be much better off following them, but that doesn't mean some aren't controversial.

Comment thread lib/teleterm/api/proto/buf.yaml Outdated
Comment thread lib/teleterm/api/protogen/js/v1/service_grpc_pb.d.ts Outdated
AntonAM
AntonAM approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@AntonAM AntonAM left a comment

Choose a reason for hiding this comment

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

Nice work! Great addition to code reliability :)

Comment thread lib/teleterm/api/protogen/golang/v1/service.pb.go Outdated
@codingllama codingllama force-pushed the codingllama/protos-teleterm branch from 29d1520 to 3ff1200 Compare August 29, 2022 15:27
@codingllama codingllama enabled auto-merge (squash) August 29, 2022 15:28
@codingllama codingllama force-pushed the codingllama/protos-teleterm branch 3 times, most recently from 8097235 to 71b9571 Compare August 29, 2022 17:52
@codingllama codingllama force-pushed the codingllama/protos-teleterm branch from 71b9571 to ec4c0a1 Compare August 29, 2022 19:19
@github-actions github-actions Bot removed the request for review from probakowski August 29, 2022 19:44
@codingllama codingllama merged commit 49e3c0d into master Aug 29, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 29, 2022

@codingllama See the table below for backport results.

Branch Result
branch/v10 Create PR

@codingllama codingllama mentioned this pull request Aug 29, 2022
Merged
@ravicious ravicious mentioned this pull request Aug 30, 2022
Open
codingllama added a commit that referenced this pull request Aug 30, 2022
@codingllama
[v10] Use Buf linters and formatter on lib/teleterm protos (#15919)
5593b46 
Similarly to #15856, moves lib/teleterm fully to Buf.

#15187

Backport #15877 to branch/v10

* Fix buf lint warnings on lib/teleterm
* Enable buf build and lint for lib/teleterm
* Use buildbox Buf in Connect, enable build/lint/format
* Reformat protos
* Update generated protos
@codingllama codingllama deleted the codingllama/protos-teleterm branch September 30, 2022 22:47
@ravicious ravicious mentioned this pull request Feb 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ravicious ravicious ravicious approved these changes

@rosstimothy rosstimothy rosstimothy approved these changes

+1 more reviewer

@AntonAM AntonAM AntonAM approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@codingllama @ravicious @AntonAM @rosstimothy