Skip to content

TCN-787 complete type filtering on buf generate#1628

Merged
elliotmjackson merged 11 commits intomainfrom
ejackson/tcn-787-complete-type-filtering-on-buf-generate
Dec 6, 2022
Merged

TCN-787 complete type filtering on buf generate#1628
elliotmjackson merged 11 commits intomainfrom
ejackson/tcn-787-complete-type-filtering-on-buf-generate

Conversation

@elliotmjackson
Copy link
Copy Markdown
Contributor

@elliotmjackson elliotmjackson commented Nov 28, 2022

Adds types attribute to the buf.gen.yaml:

version: v1
plugins:
  - name: go
    out: gen/go
+ types:
+    include:
+      - "buf.alpha.lint.v1.IDPaths"

and supply a types flag override to the cli:

Usage:
  buf generate <input> [flags]

Flags:
      --config string          The file or data to use for configuration.
      --disable-symlinks       Do not follow symlinks when reading sources or configuration from the local filesystem.
                               By default, symlinks are followed in this CLI, but never followed on the Buf Schema Registry.
      --error-format string    The format for build errors, printed to stderr. Must be one of [text,json,msvs,junit]. (default "text")
      --exclude-path strings   Exclude specific files or directories, for example "proto/a/a.proto" or "proto/a".
                               If specified multiple times, the union is taken.
  -h, --help                   help for generate
      --include-imports        Also generate all imports except for Well-Known Types.
      --include-wkt            Also generate Well-Known Types. Cannot be set without --include-imports.
  -o, --output string          The base directory to generate to. This is prepended to the out directories in the generation template. (default ".")
      --path strings           Limit to specific files or directories, for example "proto/a/a.proto" or "proto/a".
                               If specified multiple times, the union is taken.
      --template string        The generation template file or data to use. Must be in either YAML or JSON format.
+     --include-types strings     The types (message, enum, service) that should be included in this image. When specified, the resulting image will only include descriptors to describe the requested types.

resolves TCN-787
resolves TCN-788

@elliotmjackson elliotmjackson marked this pull request as ready for review November 28, 2022 16:09
@elliotmjackson elliotmjackson requested a review from jhump November 28, 2022 16:24
Copy link
Copy Markdown
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Wow, short and sweet. Glad that ended up being so succinct.

@jhump
Copy link
Copy Markdown
Member

jhump commented Nov 28, 2022

This one is probably worth adding to the change log

"",
`The file or data to use for configuration.`,
)
flagSet.StringSliceVar(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this what we want to do? We have plans to move more into the buf.gen.yaml into the future, this is going in the opposite direction. This feels like a major UI change

@akshayjshah
Copy link
Copy Markdown
Contributor

I love that this is so short. I'd imagined this being part of our managed mode configuration - is that crazy?

@jhump
Copy link
Copy Markdown
Member

jhump commented Nov 28, 2022

@akshayjshah, @bufdev, so should this be completely not supported as command-line flag? Is there any precedent for things in the yaml being overridden via flag? If so, we could start this way and then add config to the yaml that can then be overridden via this flag. WDYT?

@elliotmjackson
Copy link
Copy Markdown
Contributor Author

elliotmjackson commented Nov 28, 2022

@bufdev Going to bring your comment into this thread as i think its related.... That's a totally fair statement, shoving lists of types into the cli as flags will get messy under anything greater than very simple use.

@akshayjshah Yes, I believe that is crazy. From my point of view, "Managed Mode" has 2 distinct characteristics:

  1. It relates to file options
  2. a consumer is opting into having buf apply a set of predetermined rules in a set-and-forget fashion

Type filtering doesn't share these characteristics IMO. I believe they are more likely elevated to be equal seniority with plugins like so:

version: v1
managed:
  enabled: true
  go_package_prefix:
    default: github.com/bufbuild/buf/gen/proto/go
types:
  - buf.alpha.module.v1alpha1.Module
plugins:
  - plugin: go 
    out: gen/proto/go
    opt: paths=source_relative

@akshayjshah
Copy link
Copy Markdown
Contributor

It relates to file options

This is reasonable, but hasn't been my impression. I think of "managed mode" as all the Buf features that are effectively editing the protobuf schemas on the fly, and this feels squarely in that category. Put differently, it's all the stuff that has no equivalent in protoc.

As long as the type filters appear in the config somewhere though, I'm happy.

Comment thread private/buf/bufcli/bufcli.go Outdated
@elliotmjackson elliotmjackson marked this pull request as draft December 1, 2022 22:06
Copy link
Copy Markdown
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Cool.

Comment thread private/buf/cmd/buf/command/generate/generate.go Outdated
@elliotmjackson elliotmjackson marked this pull request as ready for review December 2, 2022 18:15
Copy link
Copy Markdown
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Just one nit in flag doc. Otherwise LGTM

Comment thread private/buf/cmd/buf/command/generate/generate.go Outdated
@elliotmjackson elliotmjackson requested a review from jhump December 6, 2022 17:58
Copy link
Copy Markdown
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

still lgtm

@elliotmjackson elliotmjackson merged commit 77d1d99 into main Dec 6, 2022
@elliotmjackson elliotmjackson deleted the ejackson/tcn-787-complete-type-filtering-on-buf-generate branch December 6, 2022 21:28
Monirul1 pushed a commit to Monirul1/buf that referenced this pull request Apr 30, 2023
Adds `types` attribute to the `buf.gen.yaml`. The types (message, enum, service) that should be included in this image. When specified, the resulting image will only include descriptors to describe the requested types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants