Use one Buf workspace instead of three #19774

ravicious · 2023-01-02T15:32:57Z

At the moment we have three different Buf workspaces, signified by buf.work.yaml files. This PR makes it so that there's only one Buf workspace with a single buf.work.yaml in the root directory.

Why

Telemetry in Connect will need to send gRPC messages that will go from the Electron app through tsh daemon to prehog. Instead of redefining each message type from lib/prehog protos in lib/teleterm, we want to import protos from lib/prehog in lib/teleterm protos.

However, in order to import protos from one module to another, both packages need to live in the same workspace.

We currently have three different buf.work.yaml files. I looked at their history and I think creating three separate workspaces wasn't a conscious choice, they just ended up this way because we never had to share stuff between them.

How

For the most part, I just had to remove those extra buf.work.yaml files and then change args to buf calls a little bit. I used bufbuild/buf-examples as the template.

I also moved lib/prehog/proto/prehog/buf.yaml to lib/prehog/proto/buf.yaml to match other modules.

There are some additional requirements Buf workspaces must meet but ours do meet them without any additional changes.

I verified that linting and formatting still works as expected across all modules. I also verified that making changes in proto files results in changes in the generated code, even for the files for which we use protoc-gen-go.

Makefile

ravicious · 2023-01-02T15:36:08Z

Makefile

@@ -1044,7 +1040,7 @@ grpc-teleterm:
 # Unlike grpc-teleterm, this target runs locally.
 .PHONY: grpc-teleterm/host
 grpc-teleterm/host: protos/all
-	cd lib/teleterm && $(BUF) generate
+	$(BUF) generate --template=lib/teleterm/buf.gen.yaml lib/teleterm/api/proto


I'm not sure but I think I should move those buf.gen.yaml files into the directories defined in buf.work.yaml.

buf.gen.yaml files used to be defined on the same level as the old buf.work.yaml. Since we removed those extra workspaces, it'd probably make more sense to have buf.gen.yaml on the same level as buf.yaml. https://docs.buf.build/configuration/v1/buf-gen-yaml

My 2c is to keep buf.gens besides the workspace file, as it is the same path were we expect codegen commands to start from.

I suggest doing away with all the additional buf.gen files and using go_package to control the output location, like we do for for everything under proto/ today. We can control whether to use protoc-gen-go or gogo in genproto.sh. (FYI @timothyb89 for prehog.)

I suggest doing away with all the additional buf.gen files and using go_package to control the output location, like we do for for everything under proto/ today. We can control whether to use protoc-gen-go or gogo in genproto.sh.

I forgot to mention that in this PR explicitly but we'll need to generate JS files from lib/prehog protos. For now I planned to maintain a separate lib/prehog/buf-teleterm.gen.yaml file.

But I suppose once we get rid of grpc-teleterm, we could have a buf-js.gen.yaml file which we'd execute for lib/prehog and lib/teleterm, right?

I'm not sure how to control the output path for the JS files without configuring that in buf.gen.yaml yet but I can tackle that once I get to it.

On top of that, lib/prehog seems to use connect-go on top of protoc-gen-go.

For now though I'd probably just keep them as they are and clean it up while removing grpc-teleterm. Does that sound good?

But I suppose once we get rid of grpc-teleterm, we could have a buf-js.gen.yaml file which we'd execute for lib/prehog and lib/teleterm, right?

That would be ideal, yes. :)

I'm not sure how to control the output path for the JS files without configuring that in buf.gen.yaml yet but I can tackle that once I get to it.

I'm not sure either. If there's no .proto option, then we might be stuck with multiple files, but that isn't that terrible. We could output both under gen/proto/js/, following suite from api/ (which I based on Buf examples), which tbh I think is even better.

We could output both under gen/proto/js/, following suite from api/ (which I based on Buf examples), which tbh I think is even better.

Ah, that's right. For some reason I was super focus on outputting them to the existing locations. But having them both under the same path might be even better due to how the generated import paths look like in JS files.

ravicious · 2023-01-02T15:39:53Z

Makefile

@@ -986,21 +986,17 @@ protos/all: protos/build protos/lint protos/format
 .PHONY: protos/build
 protos/build: buf/installed
 	$(BUF) build


If run with no additional arguments, buf commands run across all modules.

codingllama

Thanks for this, Rafal.

I think we've solved, little by little, most of the issues we had with making a single workspace, so I see no problems in merging the workspaces.

A next step could be to generate both Go and Js/Typescript in a single step, so we don't need a separate target for Teleterm. WDYT?

Makefile

codingllama · 2023-01-03T20:06:46Z

Makefile

@@ -1044,7 +1040,7 @@ grpc-teleterm:
 # Unlike grpc-teleterm, this target runs locally.
 .PHONY: grpc-teleterm/host
 grpc-teleterm/host: protos/all
-	cd lib/teleterm && $(BUF) generate
+	$(BUF) generate --template=lib/teleterm/buf.gen.yaml lib/teleterm/api/proto


My 2c is to keep buf.gens besides the workspace file, as it is the same path were we expect codegen commands to start from.

I suggest doing away with all the additional buf.gen files and using go_package to control the output location, like we do for for everything under proto/ today. We can control whether to use protoc-gen-go or gogo in genproto.sh. (FYI @timothyb89 for prehog.)

buf.work.yaml

codingllama · 2023-01-03T20:11:32Z

build.assets/genproto.sh

@@ -26,7 +27,7 @@ main() {
  cp -r github.com/gravitational/teleport/* .

  # Generate prehog protos.
-  cd lib/prehog && buf generate
+  buf generate --template=lib/prehog/buf.gen.yaml lib/prehog/proto


Could we fit prehog in one of the categories above? As in, gogo or plain protoc-gen-go?

Could we fit prehog in one of the categories above? As in, gogo or plain protoc-gen-go?

I'm going to move it to the protoc-gen-go section since it does use protoc-gen-go, but it also uses connect-go.

Right. I'm not stoked to add a 3rd proto compiler, but I won't meddle with this one either.

I think we might need a separate config - IIUC, protoc-gen-connect-go replaces protoc-gen-grpc-go.

build.assets/genproto.sh

codingllama · 2023-01-03T20:22:42Z

I also moved lib/prehog/proto/prehog/buf.yaml to lib/prehog/proto/buf.yaml to match other modules.

The new path looks correct to me - buf.yaml sits at the root of the intended proto import path.

codingllama · 2023-01-03T20:42:14Z

Also, would you mind backporting this as far as possible, so we have consistent toolchains in all active releases? I'll tentatively add the labels here.

ravicious · 2023-01-04T11:06:32Z

A next step could be to generate both Go and Js/Typescript in a single step, so we don't need a separate target for Teleterm. WDYT?

Definitely, I want to work on removing make grpc-teleterm next (#17536).

codingllama

LGTM.

I would prefer to see prehog explicitly set go_package and rely on it for the output paths, as we do with other protos. I would also prefer to not jump in on a 3rd proto compiler (connect-go) without strong reason to use it, but I understand that's how it is now. (FYI @timothyb89)

As it stands this is a good improvement over out setup, so happy to see it land.

codingllama · 2023-01-04T14:20:27Z

build.assets/genproto.sh


  # Generate protoc-gen-go protos (preferred).
  # Add your protos to the list if you can.
  buf generate --template=buf-go.gen.yaml \
    --path=api/proto/teleport/devicetrust/ \
    --path=api/proto/teleport/loginrule/ \
    --path=proto/teleport/lib/multiplexer/
+  buf generate --template=lib/prehog/buf.gen.yaml lib/prehog/proto


Possibly?

Because this is Go it still follows go_package, so we should be able to control the output path through that and have a single gen file.

Suggested change

buf generate --template=lib/prehog/buf.gen.yaml lib/prehog/proto

# Generate connect-go protos.

buf generate --template=buf-connectgo.gen.yaml lib/prehog/proto

I'll move lib/prehog/buf.gen.yaml in the PR that gets rid of make grpc-teleterm.

timothyb89 · 2023-01-04T19:37:12Z

I would also prefer to not jump in on a 3rd proto compiler (connect-go) without strong reason to use it, but I understand that's how it is now. (FYI @timothyb89)

We just used connect-go to match prehog's reference implementation, I imagine we can swap that out easily enough if it simplifies things.

github-actions · 2023-01-09T11:32:49Z

@ravicious See the table below for backport results.

Branch	Result
branch/v10	Failed
branch/v11	Failed

Use one Buf workspace instead of three

c316548

ravicious commented Jan 2, 2023

View reviewed changes

ravicious marked this pull request as ready for review January 2, 2023 15:40

github-actions bot added the size/sm label Jan 2, 2023

github-actions bot requested review from gabrielcorado and zmb3 January 2, 2023 15:40

ravicious mentioned this pull request Jan 2, 2023

Make single Buf workspace, import prehog protos #19700

Closed

ravicious added the backport/branch/v11 label Jan 2, 2023

ravicious requested a review from codingllama January 2, 2023 15:41

ravicious mentioned this pull request Jan 2, 2023

Report Connect usage events #19564

Merged

codingllama reviewed Jan 3, 2023

View reviewed changes

codingllama added the backport/branch/v10 label Jan 3, 2023

ravicious added 3 commits January 4, 2023 11:39

Remove extra calls to buf lint

24f3837

Sort workspaces alphabetically

2570da0

genproto.sh: Move lib/prehog to protoc-gen-go section

c585f09

ravicious requested a review from codingllama January 4, 2023 11:07

codingllama approved these changes Jan 4, 2023

View reviewed changes

strideynet approved these changes Jan 9, 2023

View reviewed changes

github-actions bot removed request for gabrielcorado and zmb3 January 9, 2023 10:45

Merge branch 'master' into ravicious/buf-workspace

3e20e92

ravicious enabled auto-merge (squash) January 9, 2023 11:29

ravicious merged commit 5021355 into master Jan 9, 2023

ravicious deleted the ravicious/buf-workspace branch January 9, 2023 11:33

ravicious added a commit that referenced this pull request Jan 9, 2023

Use one Buf workspace instead of three (#19774)

badb5e8

ravicious mentioned this pull request Jan 9, 2023

[v11] Use one Buf workspace instead of three #19990

Merged

ravicious added a commit that referenced this pull request Jan 9, 2023

Use one Buf workspace instead of three (#19774)

67e3b06

ravicious mentioned this pull request Jan 9, 2023

[v10] Use one Buf workspace instead of three #19993

Merged

ravicious added a commit that referenced this pull request Jan 9, 2023

Use one Buf workspace instead of three (#19774) (#19990)

dbad327

ravicious added a commit that referenced this pull request Jan 9, 2023

Use one Buf workspace instead of three (#19774) (#19993)

10366d9

ravicious mentioned this pull request Jan 10, 2023

Remove grpc-teleterm Make target and Dockerfile-teleterm #20032

Merged

ravicious mentioned this pull request Jan 27, 2023

Refactor prehog & teleterm protos to match project conventions #20810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use one Buf workspace instead of three #19774

Use one Buf workspace instead of three #19774

ravicious commented Jan 2, 2023 •

edited

Loading

ravicious Jan 2, 2023 •

edited

Loading

codingllama Jan 3, 2023

ravicious Jan 4, 2023

codingllama Jan 4, 2023

ravicious Jan 4, 2023

ravicious Jan 2, 2023

codingllama left a comment •

edited

Loading

codingllama Jan 3, 2023

codingllama Jan 3, 2023

ravicious Jan 4, 2023

codingllama Jan 4, 2023

codingllama commented Jan 3, 2023

codingllama commented Jan 3, 2023

ravicious commented Jan 4, 2023

codingllama left a comment

codingllama Jan 4, 2023

ravicious Jan 9, 2023

timothyb89 commented Jan 4, 2023

github-actions bot commented Jan 9, 2023

-  buf generate --template=lib/prehog/buf.gen.yaml lib/prehog/proto
+  # Generate connect-go protos.
+  buf generate --template=buf-connectgo.gen.yaml lib/prehog/proto

Use one Buf workspace instead of three #19774

Use one Buf workspace instead of three #19774

Conversation

ravicious commented Jan 2, 2023 • edited Loading

Why

How

ravicious Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama commented Jan 3, 2023

codingllama commented Jan 3, 2023

ravicious commented Jan 4, 2023

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyb89 commented Jan 4, 2023

github-actions bot commented Jan 9, 2023

ravicious commented Jan 2, 2023 •

edited

Loading

ravicious Jan 2, 2023 •

edited

Loading

codingllama left a comment •

edited

Loading