protobuf: During generation, copy protobuf tags back #19426
Conversation
|
Labelling this PR as size/L |
k8s-github-robot
added
the
size/L
|
Follow up from #17854 |
|
GCE e2e test build/test passed for commit 7616f1c7deed78107af5e3ef4877232f9066608d. |
| p.OmitGogo = true | ||
| } | ||
|
|
||
| // generate, but do so without gogoprotobuf extensions |
There was a problem hiding this comment.
nit, i would kill the newline above and put this command before the for loop
There was a problem hiding this comment.
I'm going to also make this optional so people can see the fully qualified IDL.
smarterclayton
force-pushed
the
rewrite_tags
branch
from
7616f1c to
6a535b8
Compare
|
Removed some round trip prototag loading for optional and repeated - it didn't actually help, and those should be controlled by the generator. |
smarterclayton
force-pushed
the
rewrite_tags
branch
from
6a535b8 to
46ea05d
Compare
|
GCE e2e test build/test passed for commit 6a535b8f793304ec630648de4f11ca823bc88acc. |
|
GCE e2e test build/test passed for commit 46ea05db21fce1d010819dd2b854da147cf2499f. |
| continue | ||
| } | ||
| if len(f.Names) > 1 { | ||
| log.Printf("WARNING: struct %s field %d %s: defined multiple names but single protobuf tag", t.Name.Name, i, f.Names[0].Name) |
There was a problem hiding this comment.
Is this supposed to cover the case where someone has a field tagged with json:"foo" yaml:"bar" ?
There was a problem hiding this comment.
Someone did "Foo, Bar string protobuf:"baz"" which means we have the same
field defined twice.
On Mon, Jan 25, 2016 at 11:13 AM, Paul Morie notifications@github.com
wrote:
In cmd/libs/go2idl/go-to-protobuf/protobuf/package.go
#19426 (comment)
:
return false- }
- switch s := t.Type.(type) {
- case *ast.StructType:
for i, f := range s.Fields.List {if len(f.Tag.Value) == 0 {continue}tag := strings.Trim(f.Tag.Value, "`")protobufTag := reflect.StructTag(tag).Get("protobuf")if len(protobufTag) == 0 {continue}if len(f.Names) > 1 {log.Printf("WARNING: struct %s field %d %s: defined multiple names but single protobuf tag", t.Name.Name, i, f.Names[0].Name)Is this supposed to cover the case where someone has a field tagged with json:"foo"
yaml:"bar" ?—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/19426/files#r50714702.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm OK if we outlaw that...
On Mon, Jan 25, 2016 at 6:46 PM, Clayton Coleman notifications@github.com
wrote:
In cmd/libs/go2idl/go-to-protobuf/protobuf/package.go
#19426 (comment)
:
return false- }
- switch s := t.Type.(type) {
- case *ast.StructType:
for i, f := range s.Fields.List {if len(f.Tag.Value) == 0 {continue}tag := strings.Trim(f.Tag.Value, "`")protobufTag := reflect.StructTag(tag).Get("protobuf")if len(protobufTag) == 0 {continue}if len(f.Names) > 1 {log.Printf("WARNING: struct %s field %d %s: defined multiple names but single protobuf tag", t.Name.Name, i, f.Names[0].Name)Someone did "Foo, Bar string
protobuf:"baz"" which means we have the
same field defined twice.
… <#-1348009057_>
On Mon, Jan 25, 2016 at 11:13 AM, Paul Morie notifications@github.com
wrote: In cmd/libs/go2idl/go-to-protobuf/protobuf/package.go <#19426
(comment)
https://github.com/kubernetes/kubernetes/pull/19426#discussion_r50714702>
: > + return false > + } > + > + switch s := t.Type.(type) { > + case
*ast.StructType: > + for i, f := range s.Fields.List { > + if
len(f.Tag.Value) == 0 { > + continue > + } > + tag :=
strings.Trim(f.Tag.Value, "`") > + protobufTag :=
reflect.StructTag(tag).Get("protobuf") > + if len(protobufTag) == 0 { > +
continue > + } > + if len(f.Names) > 1 { > + log.Printf("WARNING: struct %s
field %d %s: defined multiple names but single protobuf tag", t.Name.Name,
i, f.Names[0].Name) Is this supposed to cover the case where someone has a
field tagged with json:"foo" yaml:"bar" ? — Reply to this email directly or
view it on GitHub <
https://github.com/kubernetes/kubernetes/pull/19426/files#r50714702>.—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/19426/files#r50790202.
There was a problem hiding this comment.
smarterclayton
force-pushed
the
rewrite_tags
branch
from
46ea05d to
08fb7d7
Compare
|
Comments addressed |
|
GCE e2e test build/test passed for commit 08fb7d777cfaea54f20c9d3e889d9dea726d8f5e. |
The protobuf tags contain the assigned tag id, which then ensures subsequent generation is consistently tagging (tags don't change across generations unless someone deletes the protobuf tag). In addition, generate final proto IDL that is free of gogoproto extensions for ease of generation into other languages. Add a flag --keep-gogoproto which preserves the gogoproto extensions in the final IDL.
smarterclayton
force-pushed
the
rewrite_tags
branch
from
08fb7d7 to
14a3aaf
Compare
pmorie
added
lgtm
|
GCE e2e test build/test passed for commit 14a3aaf. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
The protobuf tags contain the assigned tag id, which then ensures
subsequent generation is consistently tagging (tags don't change across
generations unless someone deletes the protobuf tag).
In addition, generate final proto IDL that is free of gogoproto
extensions for ease of generation into other languages.
@kubernetes/sig-api-machinery