api: add java proto options to avoid risk of future breakage by dapengzhang0 · Pull Request #5764 · envoyproxy/envoy

dapengzhang0 · 2019-01-29T22:29:05Z

Description: As discussed with @snowp in envoyproxy/java-control-plane#87 (comment) and envoyproxy/java-control-plane#87 (comment), adding these options will avoid the risk of having API breaking changes on the generated java code as the proto files evolve.

Risk Level: Medium
Note that e.g. envoyproxy/java-control-plane will have to do a refactoring when they sync the proto files next time.
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue] envoyproxy/java-control-plane#87
[Optional Deprecated:]

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

snowp

Thanks! Left a few comments to get you started

snowp · 2019-01-29T22:39:04Z

tools/check_format.py

+
+def checkJavaMultipleFilesProtoOption(file_path):
+  for line in fileinput.FileInput(file_path):
+    if "option java_multiple_files = " in line:


Maybe check that this is set to true? Or are there cases where you wouldn't want that?

This only checks if the developer forgot to set it. If the developer sets it to false, that is unusual, might imply that they purposely want it to be false.

Ok, makes sense

snowp · 2019-01-29T22:40:09Z

tools/check_format.py


+def checkJavaOuterClassnameProtoOption(file_path):
+  for line in fileinput.FileInput(file_path):
+    if "option java_outer_classname = " in line:


Should this be checking for the generated name? Does that matter?

This allows purposely setting custom outer classname for some particular proto file. There's PRO and CON, but may not matter too much yet.

snowp · 2019-01-29T22:40:47Z

tools/check_format.py

+
+def fixJavaMultipleFilesProtoOption(file_path):
+  for line in fileinput.FileInput(file_path):
+    if "option java_multiple_files = " in line:


Same here, should this check for true?

Same here, allow purposely setting it to false for some particular proto file.

snowp · 2019-01-29T22:42:05Z

tools/check_format.py

+    if "option java_outer_classname = " in line:
+      return []
+
+  to_add = "option java_outer_classname = \"" + ''.join(


nit: maybe split this line into two lines to make it easier to read?

snowp · 2019-01-29T22:42:29Z

tools/check_format.py



-def checkJavaProtoOptions(file_path):
+# Add 'option java_outclass_name = FooBarProto' for foo_bar.proto


nit: typo outclass -> outer_classname

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

snowp

Looks good, thanks!

htuch

Nice, solutions that automatically catch/fix breakages like this are the best.

htuch · 2019-01-30T23:02:42Z

api/envoy/config/accesslog/v2/file.proto

 syntax = "proto3";

 package envoy.config.accesslog.v2;
+option java_outer_classname = "FileProto";


Nit: I think a blank line between package and the option block would make sense here and elsewhere.

htuch · 2019-01-30T23:03:45Z

tools/check_format.py

+    if "option java_outer_classname = " in line:
+      return []
+
+  to_add = "option java_outer_classname = \"" \


Nit: mixture of " and ` style for Python strings, please stick with the dominate convention in the file.

I'm not sure which style dominates in the file, seems both are widely used, but I changed to use only one style in my diff.

htuch · 2019-01-30T23:04:32Z

tools/check_format.py

+  if "-" in file_name or "." in file_name or not file_name.islower():
+    return ["Unable to decide java_outer_classname for proto file: %s" % file_path]
+
+  for line in fileinput.FileInput(file_path):


You don't need to iterate, just read in the file and do an in operation.

htuch · 2019-01-30T23:04:58Z

tools/check_format.py

+       + ''.join(x.title() for x in file_name.split('_')) \
+       + "Proto\";\n"
+
+  for line in fileinput.FileInput(file_path, inplace=True):


Don't need to iterate, can just do a re.sub here or similar on the entire file contents.

htuch · 2019-01-30T23:05:35Z

tools/check_format.py

+  return []
+
+
+def fixJavaMultipleFilesProtoOption(file_path):


This is similar logic to the above function, can just refactor and parameterize on the string.

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

This reverts commit 4b7bf26. Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

htuch

Thanks, looks better, a few comments.

htuch · 2019-02-04T02:35:16Z

tools/check_format.py

+    f.seek(0)
+    f.write(new_text)
+    f.truncate()
+    sys.stdout.write(new_text)


Remove debug

htuch · 2019-02-04T02:35:54Z

tools/check_format.py

+    new_text = re.sub(PROTO_PACKAGE_REGEX, repl, text)
+    f.seek(0)
+    f.write(new_text)
+    f.truncate()


Why is this needed?

htuch · 2019-02-04T02:36:28Z

tools/check_format.py

-  if package_name is None:
-    return ["Unable to find package name for proto file: %s" % file_path]
+    new_text = re.sub(PROTO_PACKAGE_REGEX, repl, text)
+    f.seek(0)


I think you could do this as two separate with blocks, one with mode r and the other with w. This would be conceptually cleaner.

htuch · 2019-02-04T02:37:05Z

tools/check_format.py

+  error_message = []
+  with open(file_path) as f:
+    result = PROTO_PACKAGE_REGEX.search(f.read())
+    if not result is None and len(result.groups()) == 1:


if result is not None and ...

htuch · 2019-02-04T02:38:11Z

tools/check_format.py

+    if pattern in f.read():
+      return []
+    return [file_path + ": " + error_message]
+    # return [] if pattern in f.read() else [file_path + ": " + error_message]


htuch · 2019-02-04T02:41:22Z

tools/check_format.py

+  if "-" in file_name or "." in file_name or not file_name.islower():
+    return ["Unable to decide java_outer_classname for proto file: %s" % file_path]
+
+  to_add = "option java_outer_classname = \"" \


FWIW, I think if you make some of these strings like option java_outer_classname = \" file-level constants, it could make it easier to keep the fix/check synced.

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

dapengzhang0 · 2019-02-04T19:36:05Z

@htuch Thanks for the additional comments. PTALA.

htuch

LGTM, but if you could fix the nit and merge master when #5827 merges, we should be able to get this to pass CI.

htuch · 2019-02-04T20:11:44Z

tools/check_format.py

-    if "option java_package = \"io.envoyproxy.envoy" in line:
-      return []
+  if not substring in text:
+


Nit: superfluous blank line.

Actually this superfluous line was suggested by check_format

$ ENVOY_DOCKER_BUILD_DIR=~/build ./ci/run_envoy_docker.sh './ci/do_ci.sh check_format' Running Python format check... --- /source/tools/check_format.py (original) +++ /source/tools/check_format.py (reformatted) @@ -157,6 +157,7 @@ text = f.read() if not substring in text: + def repl(m): return m.group(0).rstrip() + "\n\n" + to_add + "\n"

htuch · 2019-02-04T20:44:48Z

Please merge master to pick up #5827.

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

@snowp

…roxy#5764) As discussed with @snowp in [envoyproxy/java-control-plane#87 (comment)](envoyproxy/java-control-plane#87 (comment)) and [envoyproxy/java-control-plane#87 (comment)](envoyproxy/java-control-plane#87 (comment)), adding these options will avoid the risk of having API breaking changes on the generated java code as the proto files evolve. Risk Level_: Medium Note that e.g. [envoyproxy/java-control-plane](https://github.com/envoyproxy/java-control-plane) will have to do a refactoring when they sync the proto files next time. Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

dapengzhang0 added 2 commits January 29, 2019 14:18

api: add java_outer_classname and java_multiple_files proto options

f3b910a

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

run tools/check_format.py fix

4b7bf26

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

snowp self-requested a review January 29, 2019 22:32

snowp self-assigned this Jan 29, 2019

snowp suggested changes Jan 29, 2019

View reviewed changes

dapengzhang0 added 2 commits January 29, 2019 16:40

fix comments

9820f22

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

add white space

6621986

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

snowp previously approved these changes Jan 30, 2019

View reviewed changes

snowp assigned htuch Jan 30, 2019

htuch suggested changes Jan 30, 2019

View reviewed changes

dapengzhang0 added 3 commits February 1, 2019 14:25

address comments

5bbc41e

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into out…

6fc16ed

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

run check_format.py fix for a new proto file

594df02

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

dapengzhang0 dismissed snowp’s stale review via 594df02 February 1, 2019 22:57

dapengzhang0 added 2 commits February 1, 2019 15:01

Revert "run tools/check_format.py fix"

5a5c71b

This reverts commit 4b7bf26. Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

rerun check_format.py fix

7ac5c34

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

htuch suggested changes Feb 4, 2019

View reviewed changes

htuch reviewed Feb 4, 2019

View reviewed changes

dapengzhang0 added 3 commits February 4, 2019 11:23

code cleanup

8145355

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into out…

5c9afdc

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

if not result is None -> if result is not None

9824f8f

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

htuch approved these changes Feb 4, 2019

View reviewed changes

Merge branch 'master' of https://github.com/envoyproxy/envoy into out…

d29821d

…erclass Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>

htuch merged commit 4a5f858 into envoyproxy:master Feb 5, 2019

dapengzhang0 deleted the outerclass branch February 8, 2019 18:28



		def checkJavaProtoOptions(file_path):
		# Add 'option java_outclass_name = FooBarProto' for foo_bar.proto

Conversation

dapengzhang0 commented Jan 29, 2019

Uh oh!

snowp left a comment

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!

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!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

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!

dapengzhang0 commented Feb 4, 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!

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Jan 30, 2019 •

edited

Loading