Skip to content

Fix package names for v1 protos, misc proto changes#24183

Merged
espadolini merged 5 commits intomasterfrom
espadolini/proto-naming
Apr 6, 2023
Merged

Fix package names for v1 protos, misc proto changes#24183
espadolini merged 5 commits intomasterfrom
espadolini/proto-naming

Conversation

@espadolini
Copy link
Copy Markdown
Contributor

This PR:

  • adds package names following the buf managed mode convention - all imports of those files already had name overwrites, so no actual code changes are necessary, although in the future we might start referring to the packages as foov1 rather than picking some random name for them;
  • go runs protoc-gen-go and protoc-gen-go-connect directly from buf rather than via a shell script;
  • runs protogen in place rather than in github.com, as much as possible (gogoproto can't do that, so we still generate in a subdirectory and then copy the output out);
  • deletes some teleterm protogen'd files that were not supposed to exist (cc @gzdunek).

Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

This is great, Edoardo. Thanks!

I'm surprised we didn't require any code changes, I suppose that means every single "v1" import was already aliased. That's good.

Maybe rename the PR to something more descriptive? Say, "Use named packages for all v1 protos" (or something else?).

Comment thread buf-go.gen.yaml
@codingllama
Copy link
Copy Markdown
Contributor

Could we have backports too, if it's not too much trouble? Having the named packages is good if people rely on them, plus uniform codegen among release branches is nice. I'll tentatively add the labels, see what you make of it in practice.

@espadolini
Copy link
Copy Markdown
Contributor Author

I have the v12 backport ready but it's based on #24196; the other ones I can do, but I question the need for v10, since v13 is right around the corner.

@codingllama
Copy link
Copy Markdown
Contributor

I have the v12 backport ready but it's based on #24196;

Had a quick look, approved. That should make it easier. ;)

the other ones I can do, but I question the need for v10, since v13 is right around the corner.

Sure, that's fair. Your call.

@espadolini espadolini enabled auto-merge April 6, 2023 18:31
@espadolini espadolini disabled auto-merge April 6, 2023 18:31
@espadolini espadolini changed the title proto tweaks Fix package names for v1 protos, misc proto changes Apr 6, 2023
@espadolini espadolini added this pull request to the merge queue Apr 6, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 6, 2023
@espadolini espadolini added this pull request to the merge queue Apr 6, 2023
@espadolini espadolini removed this pull request from the merge queue due to a manual request Apr 6, 2023
This also adds protoc-gen-go-grpc to go.mod
@espadolini espadolini added this pull request to the merge queue Apr 6, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 6, 2023
@espadolini espadolini added this pull request to the merge queue Apr 6, 2023
Merged via the queue into master with commit 851cb65 Apr 6, 2023
@espadolini espadolini deleted the espadolini/proto-naming branch April 6, 2023 23:19
espadolini added a commit that referenced this pull request Apr 7, 2023
…24266)

* Delete teleterm's ptyHost/v1, added by mistake

* Add package name to protos conforming to PACKAGE_VERSION_SUFFIX

* use go run in buf-connect-go.gen.yaml directly

* Run protogen in place, go run in buf-go

This drops the dependency on some random v1.4.3 version of the legacy
protoc-gen-go for teleterm protos, using the version in the main go.mod
instead.
espadolini added a commit that referenced this pull request Apr 7, 2023
…24263)

* Delete teleterm's ptyHost/v1, added by mistake

* Add package name to protos conforming to PACKAGE_VERSION_SUFFIX

* use go run in buf-connect-go.gen.yaml directly

* Run protogen in place

* Run the buf-go generation off of go run

This also adds protoc-gen-go-grpc to go.mod

* go mod tidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants