Skip to content
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

Support multiple generators #45

Merged
merged 1 commit into from Oct 19, 2021
Merged

Support multiple generators #45

merged 1 commit into from Oct 19, 2021

Conversation

kzys
Copy link
Member

@kzys kzys commented Sep 23, 2021

The latest version of protoc-gen-go doesn't support gRPC. Supporting multiple generators is the way to support gRPC going forward.

@kzys kzys force-pushed the generators branch 5 times, most recently from d0ca12b to 764614f Compare September 23, 2021 17:20
@kzys kzys changed the title [WIP] Run protoc with multiple generators Support multiple generators Sep 23, 2021
@kzys kzys requested a review from stevvooe September 23, 2021 17:28
@kzys kzys marked this pull request as ready for review September 23, 2021 17:29
@kzys kzys requested a review from estesp September 23, 2021 17:30
@@ -27,10 +27,14 @@ import (
const configVersion = "unstable"

type config struct {
Version string
Generator string
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to keep compatibility with the previous syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be. Let me check.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest revision should support both Generator and Generators. The former is converted to the latter internally.

@dmcgowan dmcgowan added this to New in Code Review via automation Sep 27, 2021
@dmcgowan dmcgowan moved this from New to Ready For Review in Code Review Sep 27, 2021
@kzys kzys force-pushed the generators branch 5 times, most recently from 5aff242 to bde1693 Compare September 27, 2021 17:34
@kzys
Copy link
Member Author

kzys commented Oct 5, 2021

@AkihiroSuda @stevvooe Could you take a look?

config.go Outdated
Plugins []string
Includes struct {

// Plugins will be deprecated. It has to be per-Generator setting, but neither protoc-gen-go nor protoc-gen-go-grpc
Copy link
Member

Choose a reason for hiding this comment

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

The statement about plugin support is a bit hard to understand with the double negative. Is this saying both support plugins?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. That was simply wrong. It has to be

but neither protoc-gen-go nor protoc-gen-go-grpc support plugins

The latest version of protoc-gen-go doesn't support gRPC. Supporting
multiple generators is the way to support gRPC going forward.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Copy link
Member

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

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

LGTM

Is it time to start versioning the config?

@stevvooe stevvooe merged commit b7ebc4c into containerd:main Oct 19, 2021
Code Review automation moved this from Ready For Review to Done Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants