Automated protobuf generation #30

howardjohn · 2020-01-10T02:06:35Z

Fixes #11

k8s-ci-robot · 2020-01-10T02:06:44Z

Hi @howardjohn. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bowei · 2020-01-10T17:33:46Z

/ok-to-test

howardjohn · 2020-01-29T18:59:14Z

@bowei do you have a chance to take a look? This is blocking Istio adoption - right now we are using a fork with these changes

robscott · 2020-02-06T18:40:31Z

@howardjohn We discussed this on Service APIs call and want to move forward, this would be great with verification and generation scripts mirroring what exists in k/k.

howardjohn · 2020-02-10T17:35:44Z

Sorry for the delay here. I have updated the PR to rebase and add a verification script. Looks like we need a new prow job as well, I can send a PR for that

bowei · 2020-02-11T22:25:03Z

/assign @robscott

robscott

Thanks for your work on this! This looks really close but had a few nits/questions. One question that didn't really fit with any single line of code is simply what would happen if a field was added or removed from a struct - would the int associated with the protobuf tag for fields below that field in a struct change? If so, would that break things?

robscott · 2020-02-12T01:33:56Z

kubebuilder.mk

@@ -17,6 +17,10 @@ IMG ?= controller:latest
 # Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
 CRD_OPTIONS ?= "crd:trivialVersions=true"

+DOCKER ?= docker
+# Image to build protobugs
+PROTO_IMG ?= k8s.gcr.io/kube-cross:v1.13.5-1


It looks like there's a slightly newer version of this image now, any reason not to use it?

It didn't exist at the time, I'll update it

kubebuilder.mk

robscott · 2020-02-12T01:41:03Z

hack/verify-kubebuilder.sh

@@ -0,0 +1,31 @@
+#!/bin/bash


Why is this called verify-kubebuilder.sh? Isn't this more about just verifying proto generation?

I figured we could extend this to verify everything in kubebuilder, but I can make this proto specific

👍 that makes sense. I think I'd prefer a proto specific name for the file to better match the other verify-* script names. I may be misunderstanding this, but isn't proto generation unrelated to kubebuilder, and instead something we're adding on top with go-to-protobuf?

That is a good point.. the kubebuild.mk also has format, vet, etc which aren't quite kubebuilder either which is why I placed this here. For now ill change the verify script

Yeah, that's also a good point, confusing naming.

howardjohn · 2020-02-12T17:57:15Z

Thanks for your work on this! This looks really close but had a few nits/questions. One question that didn't really fit with any single line of code is simply what would happen if a field was added or removed from a struct - would the int associated with the protobuf tag for fields below that field in a struct change? If so, would that break things?

I just tested this, removing a field and regen, no numbers change. If we then add another field, that one is given a new number, it does not re-use the old one. So I think this is working as expected in that regard

howardjohn · 2020-02-12T19:21:56Z

/retest

robscott · 2020-02-12T19:55:47Z

Thanks!

/lgtm

howardjohn · 2020-02-13T01:20:41Z

Thanks @bowei I have updated with your suggestions

howardjohn · 2020-02-18T19:37:20Z

@bowei this is ready for review

bowei · 2020-02-18T20:49:40Z

/ok-to-test

hack/verify-proto.sh

howardjohn · 2020-02-19T23:00:22Z

I set up the prow job in kubernetes/test-infra#16379 which can be enabled after this is merged

howardjohn · 2020-02-25T00:03:03Z

@bowei do you have a chance to take a look at this today?

bowei · 2020-02-26T23:05:02Z

/lgtm
/approve

howardjohn · 2020-02-27T00:46:14Z

I have updated this to resolve some merge conflicts

bowei · 2020-02-27T17:23:10Z

/lgtm
/approve

k8s-ci-robot · 2020-02-27T17:23:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, howardjohn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

howardjohn · 2020-02-27T17:29:44Z

We will probably want kubernetes/test-infra#16379 or we may end up with some out of sync issues

Miciah · 2020-02-27T19:14:57Z

api/v1alpha1/gateway_types.go

 type CertificateObjectReference = LocalObjectReference

 // ListenerExtensionObjectReference identifies a listener extension object
 // within a known namespace.
 //
 // +k8s:deepcopy-gen=false
+// +protobuf=false
 type ListenerExtensionObjectReference = LocalObjectReference

 // RouteObjectReference identifies a route object within a known namespace.


Should this have +protobuf=false too? I'm trying to rebase #111 and getting failures:

api/v1alpha1/generated.pb.go:1684:33: m.Routes[iNdEx].MarshalToSizedBuffer undefined (type RouteObjectReference has no field or method MarshalToSizedBuffer) api/v1alpha1/generated.pb.go:3155:9: e.Size undefined (type RouteObjectReference has no field or method Size) api/v1alpha1/generated.pb.go:3758:63: f.String undefined (type RouteObjectReference has no field or method String) api/v1alpha1/generated.pb.go:5705:21: cannot use LocalObjectReference literal (type LocalObjectReference) as type RouteObjectReference in append api/v1alpha1/generated.pb.go:5706:39: m.Routes[len(m.Routes) - 1].Unmarshal undefined (type RouteObjectReference has no field or method Unmarshal)

My understanding is that Go associates methods with the concrete types, not the alias. So we cannot have ListenerExtensionObjectReference.DeepCopy and RouteObjectReference.DeepCopy, since then there are two DeepCopy associated with LocalObjectReference. The go-to-protobuf generator doesn't seem to handle this well. RouteObjectReference is somehow deemed as the chosen alias that represents LocalObjectReference (possibly its the last one processed?), so if we mark it protobuf=false there will be no type for LocalObjectReference generated.

Since you are making RouteObjectReference no longer a type, it should have protobuf generated, but now there is probably nothing generating LocalObjectReference? So maybe we need to remove protobuf=false from one of these type aliases now?

Let me know if that makes sense, I can try to get things working on your PR if not

That makes sense. Thanks!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 10, 2020

k8s-ci-robot requested review from bowei and thockin January 10, 2020 02:06

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2020

howardjohn force-pushed the proto-auto branch 2 times, most recently from 1301c93 to 8204111 Compare January 14, 2020 16:38

howardjohn force-pushed the proto-auto branch from 8204111 to 4c8ab34 Compare January 22, 2020 17:44

howardjohn marked this pull request as ready for review January 22, 2020 17:45

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2020

howardjohn requested a review from robscott January 23, 2020 20:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2020

howardjohn force-pushed the proto-auto branch from 4c8ab34 to de40610 Compare February 10, 2020 17:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020

k8s-ci-robot assigned robscott Feb 11, 2020

robscott reviewed Feb 12, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

howardjohn force-pushed the proto-auto branch 2 times, most recently from 0ec66c2 to 92859a8 Compare February 18, 2020 18:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2020

bowei reviewed Feb 18, 2020

View reviewed changes

hack/verify-proto.sh Outdated Show resolved Hide resolved

howardjohn mentioned this pull request Feb 19, 2020

Add verify-proto job for service-apis kubernetes/test-infra#16379

Closed

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2020

howardjohn added 4 commits February 26, 2020 16:37

Automated protobuf generation

9007402

Use docker for verify-proto

d0bec93

Proper generation of protos

8713687

Fix type aliases

db5f0b9

howardjohn force-pushed the proto-auto branch from 18807e3 to db5f0b9 Compare February 27, 2020 00:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2020

k8s-ci-robot merged commit b9010cf into kubernetes-sigs:master Feb 27, 2020

Miciah reviewed Feb 27, 2020

View reviewed changes

Miciah mentioned this pull request Feb 28, 2020

Delete RouteObjectReference; make group optional; add godoc #111

Merged

danehans mentioned this pull request Apr 26, 2020

Makefile improvements #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated protobuf generation #30

Automated protobuf generation #30

howardjohn commented Jan 10, 2020 •

edited

Loading

k8s-ci-robot commented Jan 10, 2020

bowei commented Jan 10, 2020

howardjohn commented Jan 29, 2020

robscott commented Feb 6, 2020

howardjohn commented Feb 10, 2020

bowei commented Feb 11, 2020

robscott left a comment

robscott Feb 12, 2020

howardjohn Feb 12, 2020

robscott Feb 12, 2020

howardjohn Feb 12, 2020

robscott Feb 12, 2020

howardjohn Feb 12, 2020

robscott Feb 12, 2020

howardjohn commented Feb 12, 2020

howardjohn commented Feb 12, 2020

robscott commented Feb 12, 2020

howardjohn commented Feb 13, 2020

howardjohn commented Feb 18, 2020

bowei commented Feb 18, 2020

howardjohn commented Feb 19, 2020

howardjohn commented Feb 25, 2020

bowei commented Feb 26, 2020

howardjohn commented Feb 27, 2020

bowei commented Feb 27, 2020

k8s-ci-robot commented Feb 27, 2020

howardjohn commented Feb 27, 2020

Miciah Feb 27, 2020

howardjohn Feb 27, 2020

Miciah Feb 27, 2020

Automated protobuf generation #30

Automated protobuf generation #30

Conversation

howardjohn commented Jan 10, 2020 • edited Loading

k8s-ci-robot commented Jan 10, 2020

bowei commented Jan 10, 2020

howardjohn commented Jan 29, 2020

robscott commented Feb 6, 2020

howardjohn commented Feb 10, 2020

bowei commented Feb 11, 2020

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Feb 12, 2020

howardjohn commented Feb 12, 2020

robscott commented Feb 12, 2020

howardjohn commented Feb 13, 2020

howardjohn commented Feb 18, 2020

bowei commented Feb 18, 2020

howardjohn commented Feb 19, 2020

howardjohn commented Feb 25, 2020

bowei commented Feb 26, 2020

howardjohn commented Feb 27, 2020

bowei commented Feb 27, 2020

k8s-ci-robot commented Feb 27, 2020

howardjohn commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Jan 10, 2020 •

edited

Loading