Skip to content

Use one Buf workspace instead of three #19774

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.

Sign up for GitHub

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

Merged
ravicious merged 5 commits into from
Jan 9, 2023
Merged

Use one Buf workspace instead of three #19774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c316548
Use one Buf workspace instead of three
ravicious Dec 28, 2022
24f3837
Remove extra calls to buf lint
ravicious Jan 4, 2023
2570da0
Sort workspaces alphabetically
ravicious Jan 4, 2023
c585f09
genproto.sh: Move lib/prehog to protoc-gen-go section
ravicious Jan 4, 2023
3e20e92
Merge branch 'master' into ravicious/buf-workspace
ravicious Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,21 +987,15 @@ protos/all: protos/build protos/lint protos/format
.PHONY: protos/build
protos/build: buf/installed
$(BUF) build
Copy link
Copy Markdown
Member Author

@ravicious ravicious Jan 2, 2023

Choose a reason for hiding this comment

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

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

cd lib/teleterm && $(BUF) build
cd lib/prehog && $(BUF) build

.PHONY: protos/format
protos/format: buf/installed
$(BUF) format -w
cd lib/teleterm && $(BUF) format -w
cd lib/prehog && $(BUF) format -w

.PHONY: protos/lint
protos/lint: buf/installed
$(BUF) lint
cd api/proto && $(BUF) lint --config=buf-legacy.yaml
cd lib/teleterm && $(BUF) lint
cd lib/prehog && $(BUF) lint
$(BUF) lint --config=api/proto/buf-legacy.yaml api/proto

.PHONY: lint-protos
lint-protos: protos/lint
Expand Down Expand Up @@ -1045,7 +1039,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
Copy link
Copy Markdown
Member Author

@ravicious ravicious Jan 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@codingllama codingllama Jan 3, 2023

Choose a reason for hiding this comment

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

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.)

Copy link
Copy Markdown
Member Author

@ravicious ravicious Jan 4, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@codingllama codingllama Jan 4, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@ravicious ravicious Jan 4, 2023

Choose a reason for hiding this comment

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

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.


.PHONY: goinstall
goinstall:
Expand Down
2 changes: 2 additions & 0 deletions buf.work.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
version: v1
directories:
- api/proto
- lib/prehog/proto
- lib/teleterm/api/proto
- proto
9 changes: 4 additions & 5 deletions build.assets/genproto.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ main() {
trap 'rm -fr github.com' EXIT # don't leave github.com/ behind
rm -fr api/gen/proto gen/proto # cleanup gen/proto folders

# Generate Gogo protos (default).
buf generate --template=buf-gogo.gen.yaml
# Generate Gogo protos.
buf generate --template=buf-gogo.gen.yaml api/proto
buf generate --template=buf-gogo.gen.yaml proto
Comment thread
codingllama marked this conversation as resolved.

# Generate protoc-gen-go protos (preferred).
# Add your protos to the list if you can.
Expand All @@ -23,11 +24,9 @@ main() {
--path=api/proto/teleport/loginrule/ \
--path=api/proto/teleport/proxy/ \
--path=proto/teleport/lib/multiplexer/
buf generate --template=lib/prehog/buf.gen.yaml lib/prehog/proto
Copy link
Copy Markdown
Contributor

@codingllama codingllama Jan 4, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@ravicious ravicious Jan 9, 2023

Choose a reason for hiding this comment

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

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


cp -r github.com/gravitational/teleport/* .

# Generate prehog protos.
cd lib/prehog && buf generate
}

main "$@"
8 changes: 4 additions & 4 deletions lib/prehog/buf.gen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ managed:
default: github.com/gravitational/teleport/lib/prehog/gen
plugins:
- name: go
path: bin/protoc-gen-go
out: gen
path: lib/prehog/bin/protoc-gen-go
out: lib/prehog/gen
opt:
- paths=source_relative
- name: connect-go
path: bin/protoc-gen-connect-go
out: gen
path: lib/prehog/bin/protoc-gen-connect-go
out: lib/prehog/gen
opt:
- paths=source_relative
3 changes: 0 additions & 3 deletions lib/prehog/buf.work.yaml
View file
Open in desktop

This file was deleted.

0 lib/prehog/proto/prehog/buf.yaml → lib/prehog/proto/buf.yaml
View file
Open in desktop
File renamed without changes.
10 changes: 5 additions & 5 deletions lib/teleterm/buf.gen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
version: v1
plugins:
- name: go
out: api/protogen/golang
out: lib/teleterm/api/protogen/golang
opt:
- paths=source_relative

- name: go-grpc
out: api/protogen/golang
out: lib/teleterm/api/protogen/golang
opt:
- paths=source_relative

- name: js
out: api/protogen/js
out: lib/teleterm/api/protogen/js
opt:
- import_style=commonjs,binary

- name: grpc
out: api/protogen/js
out: lib/teleterm/api/protogen/js
opt: grpc_js
path: grpc_tools_node_protoc_plugin

- name: ts
out: api/protogen/js
out: lib/teleterm/api/protogen/js
opt: "service=grpc-node"
3 changes: 0 additions & 3 deletions lib/teleterm/buf.work.yaml
View file
Open in desktop

This file was deleted.