api: add go proto generation script by kyessenov · Pull Request #8155 · envoyproxy/envoy

kyessenov · 2019-09-05T03:30:08Z

Signed-off-by: Kuat Yessenov kuat@google.com

Description:
Adds a script to create a go module from the generated protobufs as part of #8151.
The module appears to build with the following module declaration:

module github.com/envoyproxy/data-plane-api/api

go 1.12

require (
        github.com/census-instrumentation/opencensus-proto v0.2.1
        github.com/envoyproxy/protoc-gen-validate v0.1.0
        github.com/gogo/protobuf v1.3.0
        github.com/golang/protobuf v1.3.2
        github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
        google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
        google.golang.org/grpc v1.23.0
)

Add CI automation to trigger the script after the merge to master in envoyproxy.

Risk Level: low
Testing: local build
Docs Changes: none
Release Notes: none
Fixes #8151

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2019-09-05T03:38:37Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8155 was synchronize by kyessenov.

see: more, trace.

nareddyt · 2019-09-05T05:43:47Z

Correct me if I'm wrong, but this will only generate gogo/protobuf protos? Is there any way we can leverage the existing bazel build targets to generate files compatible with golang/protobuf and check them both into data-plane-api?

If not, this basically forces all dependent golang code to use gogo. That would not work for grpc-go nor the goproto-based go-control-plane described in envoyproxy/go-control-plane#213.

kyessenov · 2019-09-05T05:46:59Z

No, these are golang/protobuf protos as they are produced by bazel. The dependency on gogoproto is a vanity import due to the annotations. I don't think it does anything at runtime. We should just clean-up these annotations at some point.

htuch

Thanks; I think this approach of relying the bazel build and scraping is a reasonable way to go, we do it for docs.

It might be possible to make it even cleaner via a final genrule that collects all the .go in a zip and then just do a copy+unzip here if you want to do more Bazel plumbing
/wait.

tools/api/generate_go_protobuf.py

htuch · 2019-09-05T18:41:33Z

tools/api/generate_go_protobuf.py

+  # go_out/envoy/config/bootstrap/v2
+  rule_dir, proto = rule[len("@envoy_api//"):].rsplit(':', 1)
+
+  input_dir = os.path.join(bazel_bin, 'external', 'envoy_api', rule_dir, 'linux_amd64_stripped',


This code is quite similar to https://github.com/envoyproxy/envoy/blob/master/docs/build.sh#L66. Is there a way to refactor to share, or maybe you can avoid reaching into amd64 named dirs somehow like we do for docs?

linux_amd64_stripped is specific to rules_go I think. It's not coming from bazel itself. I've tried to figure out how to get generated files with bazel query but eventually gave up.

Yes, it's due to compilation modes in go. It statically links everything, so it has to keep outputs in sub-directories https://github.com/bazelbuild/rules_go/blob/0303b3a69695e35940b09ddbf7a444bcc7fbefd4/go/private/mode.bzl.

Can you specify --strip=always or -c fastbuild when bazel build is invoked in line 25? Otherwise the directory name is interfered by .bazelrc (e.g. I have -c dbg in my bazelrc)

Maybe it's better to recommend running this script with empty --bazelrc=/dev/null on linux amd64?
If you poke around the logic for the choice of the directory, it's pretty convoluted and interferes with msan/race/etc: https://github.com/bazelbuild/rules_go/blob/0303b3a69695e35940b09ddbf7a444bcc7fbefd4/go/private/mode.bzl#L135

yeah an empty bazelrc works too but I think that ignores workspace .bazelrc

ok, -c fastbuild is better than ignoring workspace bazel rc.

htuch · 2019-09-05T18:41:57Z

tools/api/generate_go_protobuf.py

+    os.makedirs(output_dir, 0o755)
+  for generated_file in input_files:
+    shutil.copy(generated_file, output_dir)
+    os.chmod(os.path.join(output_dir, generated_file), 0o644)


You could optionally just call rsync here.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-09-05T20:52:06Z

Re: final genrule: there's an experimental go_path rule to do just that (https://github.com/bazelbuild/rules_go/blob/0303b3a69695e35940b09ddbf7a444bcc7fbefd4/go/core.rst#go-path). We should probably switch to that when it's ready.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-09-06T01:20:03Z

Mirrored stubs are available in https://github.com/envoyproxy/go-control-plane/tree/v2

nareddyt · 2019-09-06T04:41:27Z

This is great! Just wondering why you ended up placing the generated protos in go-control-plane instead of data-plane-api?

kyessenov · 2019-09-06T04:44:10Z

go-control-plane has a CI system for validation and it is not read-only. It also sounded like less work than creating a brand new repository.

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan · 2019-09-09T19:11:25Z

tools/api/generate_go_protobuf.py

+
+# Each rule has the form @envoy_api//foo/bar:baz_go_proto.
+# First build all the rules to ensure we have the output files.
+if call(['bazel', 'build', '-c', 'fastbuild'] + go_protos) != 0:


just use subprocess.check_call?

lizan

LGTM, defer to @envoyproxy/api-shepherds

kyessenov · 2019-09-11T05:44:29Z

Let me extend this script to do mirroring as well. Would be nice to have a self-contained tool to both generate and publish.

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2019-09-11T19:30:39Z

@kyessenov can this wait until @htuch gets back from vacation next week? He is better at Python and Bazel than I am.

kyessenov · 2019-09-11T20:06:43Z

Sure, I'm refining the auto-sync script and will wait for Harvey to review it.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-09-12T17:53:12Z

This should be ready to after review.
If you grant the circle keys write access to go-control-plane/master, it should start publishing content on every change to api/ marked with the SHA of the change.

htuch

Looks great, a few comments/questions.

tools/api/generate_go_protobuf.py

htuch · 2019-09-16T22:30:16Z

api/bazel/api_build_system.bzl

 _CC_EXPORT_SUFFIX = "_export_cc"
 _GO_PROTO_SUFFIX = "_go_proto"
-_GO_IMPORTPATH_PREFIX = "github.com/envoyproxy/data-plane-api/api/"
+_GO_IMPORTPATH_PREFIX = "github.com/envoyproxy/go-control-plane/"


How does this impact local development flows?

This aligns the generated/bazel-internal build with the public bazel-less build, so I think it's an improvement.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-09-17T02:13:34Z

I'd like to keep this script simple and straightforward. Bash would be fine, but I can never really understand the control flow logic in it. I don't think we want a framework for the python scripts. This is a dangerous path to fall in (see istio's 5+ test frameworks for running scripts).

htuch

/wait

htuch · 2019-09-18T02:23:24Z

tools/api/generate_go_protobuf.py

+REPO_BASE = 'go-control-plane'
+BRANCH = 'master'
+MIRROR_MSG = 'Mirrored from envoyproxy/envoy @ '
+USER_NAME = 'Kuat Yessenov'


I think we should have some kind of robo account here, e.g. what we do in https://github.com/envoyproxy/envoy/blob/master/ci/api_mirror.sh#L13.

Is this a real account or just random email/username? I couldn't tell, and DCO didn't like it.

Switched to a robo account.

tools/api/generate_go_protobuf.py

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

Thanks! LGTM

Adds a script to create a go module from the generated protobufs as part of envoyproxy#8151. The module appears to build with the following module declaration: module github.com/envoyproxy/data-plane-api/api go 1.12 require ( github.com/census-instrumentation/opencensus-proto v0.2.1 github.com/envoyproxy/protoc-gen-validate v0.1.0 github.com/gogo/protobuf v1.3.0 github.com/golang/protobuf v1.3.2 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4 google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55 google.golang.org/grpc v1.23.0 ) Add CI automation to trigger the script after the merge to master in envoyproxy. Risk Level: low Testing: local build Docs Changes: none Release Notes: none Fixes envoyproxy#8151 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 2 commits September 4, 2019 20:27

go proto generation script

7296975

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix python format

494ecd1

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch self-assigned this Sep 5, 2019

htuch suggested changes Sep 5, 2019

View reviewed changes

kyessenov added 2 commits September 5, 2019 13:41

review feedback

6bea765

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

55cd052

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 4 commits September 5, 2019 15:25

fix format

41e2254

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

4ab0339

Signed-off-by: Kuat Yessenov <kuat@google.com>

update script

0b8c0df

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix test file

b692406

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan self-assigned this Sep 6, 2019

kyessenov added 2 commits September 6, 2019 16:25

review feedback

7745cca

Signed-off-by: Kuat Yessenov <kuat@google.com>

update package import

6a40cc0

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan reviewed Sep 9, 2019

View reviewed changes

kyessenov mentioned this pull request Sep 10, 2019

Transition to use golang/protobuf to support automated code generation envoyproxy/go-control-plane#226

Merged

Merge remote-tracking branch 'upstream/master' into generate_go_protos

cd50630

lizan previously approved these changes Sep 11, 2019

View reviewed changes

adding sync

07bba3b

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed lizan’s stale review via 07bba3b September 11, 2019 06:39

format

790fa31

Signed-off-by: Kuat Yessenov <kuat@google.com>

nareddyt mentioned this pull request Sep 11, 2019

Proposal: Check-in generated golang protos in envoy/api #8151

Closed

mattklein123 self-assigned this Sep 11, 2019

mattklein123 removed their assignment Sep 11, 2019

mattklein123 added the waiting label Sep 11, 2019

update script

d3bdc13

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Sep 11, 2019

kyessenov added 3 commits September 11, 2019 17:06

update the script

85923b3

Signed-off-by: Kuat Yessenov <kuat@google.com>

hook it up

e415f56

Signed-off-by: Kuat Yessenov <kuat@google.com>

update branch

cf27aa1

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch suggested changes Sep 16, 2019

View reviewed changes

kyessenov added 2 commits September 16, 2019 16:52

Merge remote-tracking branch 'upstream/master' into generate_go_protos

91dacaa

review feedback

bdf73b3

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch suggested changes Sep 18, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 18, 2019

kyessenov added 2 commits September 18, 2019 09:42

Merge remote-tracking branch 'upstream/master' into generate_go_protos

beee205

review

f0e736e

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Sep 18, 2019

fix format

21f10e7

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch approved these changes Sep 19, 2019

View reviewed changes

htuch merged commit 32e6018 into envoyproxy:master Sep 19, 2019

Conversation

kyessenov commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 5, 2019

Uh oh!

nareddyt commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Sep 5, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 5, 2019

Uh oh!

kyessenov commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nareddyt commented Sep 6, 2019

Uh oh!

kyessenov commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 11, 2019

Uh oh!

mattklein123 commented Sep 11, 2019

Uh oh!

kyessenov commented Sep 11, 2019

Uh oh!

kyessenov commented Sep 12, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 17, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kyessenov commented Sep 5, 2019 •

edited

Loading

nareddyt commented Sep 5, 2019 •

edited

Loading

kyessenov commented Sep 6, 2019 •

edited

Loading

kyessenov commented Sep 6, 2019 •

edited

Loading

kyessenov Sep 18, 2019 •

edited

Loading