From ee1782f8f4dd4e8db024cbd3b6105bb52e67942e Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 14 Jul 2021 15:37:07 +0000 Subject: [PATCH 01/16] Add buf bazel dependency and tests to evaluate it Signed-off-by: Yaseen Alkhafaji --- bazel/external/api_buf.BUILD | 11 ++ bazel/repositories.bzl | 9 ++ bazel/repository_locations.bzl | 12 ++ .../api_proto_breaking_change_detector/BUILD | 67 +++++++++ .../buf.lock | 45 ++++++ .../buf.yaml | 23 ++++ .../detector.py | 129 ++++++++++++++++++ .../detector_test.py | 103 ++++++++++++++ .../api_proto_breaking_change_detector/BUILD | 36 +++++ .../allowed/test_add_comment_current | 9 ++ .../allowed/test_add_comment_next | 10 ++ .../allowed/test_add_enum_value_current | 17 +++ .../allowed/test_add_enum_value_next | 18 +++ .../allowed/test_add_field_current | 6 + .../allowed/test_add_field_next | 7 + .../allowed/test_add_option_current | 9 ++ .../allowed/test_add_option_next | 10 ++ .../test_force_breaking_change_current | 6 + .../allowed/test_force_breaking_change_next | 6 + .../test_remove_and_reserve_field_current | 7 + .../test_remove_and_reserve_field_next | 9 ++ .../test_change_field_from_oneof_current | 10 ++ .../test_change_field_from_oneof_next | 10 ++ .../breaking/test_change_field_id_current | 6 + .../breaking/test_change_field_id_next | 6 + .../breaking/test_change_field_name_current | 6 + .../breaking/test_change_field_name_next | 6 + .../test_change_field_plurality_current | 6 + .../breaking/test_change_field_plurality_next | 6 + .../test_change_field_to_oneof_current | 10 ++ .../breaking/test_change_field_to_oneof_next | 10 ++ .../breaking/test_change_field_type_current | 6 + .../breaking/test_change_field_type_next | 6 + .../breaking/test_change_package_name_current | 6 + .../breaking/test_change_package_name_next | 6 + .../breaking/test_change_pgv_field_current | 6 + .../breaking/test_change_pgv_field_next | 6 + .../breaking/test_change_pgv_message_current | 8 ++ .../breaking/test_change_pgv_message_next | 8 ++ .../breaking/test_change_pgv_oneof_current | 10 ++ .../breaking/test_change_pgv_oneof_next | 10 ++ 41 files changed, 697 insertions(+) create mode 100644 bazel/external/api_buf.BUILD create mode 100644 tools/api_proto_breaking_change_detector/BUILD create mode 100644 tools/api_proto_breaking_change_detector/buf.lock create mode 100644 tools/api_proto_breaking_change_detector/buf.yaml create mode 100644 tools/api_proto_breaking_change_detector/detector.py create mode 100644 tools/api_proto_breaking_change_detector/detector_test.py create mode 100644 tools/testdata/api_proto_breaking_change_detector/BUILD create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_next create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current create mode 100644 tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next diff --git a/bazel/external/api_buf.BUILD b/bazel/external/api_buf.BUILD new file mode 100644 index 0000000000000..dfab37624106c --- /dev/null +++ b/bazel/external/api_buf.BUILD @@ -0,0 +1,11 @@ +package( + default_visibility = ["//visibility:public"], +) + +filegroup( + name = "buf", + srcs = [ + "@com_github_bufbuild_buf//:bin/buf", + ], + tags = ["manual"], +) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index be758fa4cebfd..87d7ef7b30bcc 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -192,6 +192,8 @@ def envoy_dependencies(skip_targets = []): _com_github_wasmtime() _com_github_wasm_c_api() + _com_github_bufbuild_buf() + switched_rules_by_language( name = "com_google_googleapis_imports", cc = True, @@ -1024,6 +1026,13 @@ filegroup( build_file_content = BUILD_ALL_CONTENT, ) +def _com_github_bufbuild_buf(): + external_http_archive( + name = "com_github_bufbuild_buf", + build_file = "@envoy//bazel/external:api_buf.BUILD", + tags = ["manual"], + ) + def _foreign_cc_dependencies(): external_http_archive("rules_foreign_cc") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 2b2b53eba6688..c54318cfd9451 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1072,4 +1072,16 @@ REPOSITORY_LOCATIONS_SPEC = dict( release_date = "2018-12-18", cpe = "N/A", ), + com_github_bufbuild_buf = dict( + project_name = "buf", + project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs + project_url = "https://buf.build", + version = "0.46.0", + sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", + strip_prefix = "buf", + urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], + release_date = "2021-07-27", + use_category = ["api"], + tags = ["manual"], + ), ) diff --git a/tools/api_proto_breaking_change_detector/BUILD b/tools/api_proto_breaking_change_detector/BUILD new file mode 100644 index 0000000000000..9b6a50ffed08e --- /dev/null +++ b/tools/api_proto_breaking_change_detector/BUILD @@ -0,0 +1,67 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_test") + +licenses(["notice"]) # Apache 2 + +py_binary( + name = "proto_breaking_change_detector", + srcs = ["detector.py"], + data = [ + "//tools/api_proto_breaking_change_detector:buf.lock", + "//tools/api_proto_breaking_change_detector:buf.yaml", + "@com_github_bufbuild_buf//:buf", + ], + main = "detector.py", + tags = ["manual"], + deps = [ + "//tools:run_command", + ], +) + +py_test( + name = "proto_breaking_change_detector_test", + srcs = ["detector_test.py"], + data = [ + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_comment_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_comment_next", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_enum_value_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_enum_value_next", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_field_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_field_next", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_option_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_option_next", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_force_breaking_change_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_force_breaking_change_next", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_remove_and_reserve_field_current", + "//tools/testdata/api_proto_breaking_change_detector:allowed/test_remove_and_reserve_field_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_from_oneof_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_from_oneof_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_id_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_id_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_name_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_name_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_plurality_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_plurality_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_to_oneof_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_to_oneof_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_type_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_type_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_package_name_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_package_name_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_field_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_field_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_message_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_message_next", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_oneof_current", + "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_oneof_next", + "@com_github_bufbuild_buf//:buf", + ], + main = "detector_test.py", + python_version = "PY3", + srcs_version = "PY3", + tags = ["manual"], + visibility = ["//visibility:public"], + deps = [ + ":proto_breaking_change_detector", + "@rules_python//python/runfiles", + ], +) diff --git a/tools/api_proto_breaking_change_detector/buf.lock b/tools/api_proto_breaking_change_detector/buf.lock new file mode 100644 index 0000000000000..d33cf64e43a58 --- /dev/null +++ b/tools/api_proto_breaking_change_detector/buf.lock @@ -0,0 +1,45 @@ +# Generated by buf. DO NOT EDIT. +version: v1 +deps: + - remote: buf.build + owner: beta + repository: googleapis + branch: main + commit: 1c473ad9220a49bca9320f4cc690eba5 + digest: b1-unlhrcI3tnJd0JEGuOb692LZ_tY_gCGq6mK1bgCn1Pg= + create_time: 2021-06-23T20:16:47.788079Z + - remote: buf.build + owner: beta + repository: opencensus + branch: main + commit: 5f5f8259293649d68707d2e5b6285748 + digest: b1-myYwcdM0Xu05qIwhiy4eWEcARYUuZZ1vTbYvrrHu1mU= + create_time: 2021-03-03T20:50:42.079743Z + - remote: buf.build + owner: beta + repository: opentelemetry + branch: main + commit: ee82722300c04a618e1c9a2373ce2958 + digest: b1-MwMH-u3ygagogGt6GnJMsqSyxDL-6RKCThaBQRdT_28= + create_time: 2021-06-23T20:17:01.385563Z + - remote: buf.build + owner: beta + repository: prometheus + branch: main + commit: a91b42d18a994cd4b07b37f365f87cf9 + digest: b1-uKmv58fyoNwJI855qg7UEagfdyUl6XNPsFAdDoi57f4= + create_time: 2021-06-23T20:16:58.410272Z + - remote: buf.build + owner: beta + repository: protoc-gen-validate + branch: main + commit: 3aa3a142febe4d198171026cddadb18b + digest: b1-MkZlf7zGl1xvJCsBhk-Kh46tvCHpRNpGNUOtryA9-ng= + create_time: 2021-03-30T19:57:01.168017Z + - remote: buf.build + owner: beta + repository: udpa + branch: main + commit: 64457162aab743c5a695a8cccbd93d8d + digest: b1-15-sblwZI7jDyUli6FyJsBTy8dRe6mHTA2B2VTcIm4g= + create_time: 2021-03-30T19:57:09.877302Z diff --git a/tools/api_proto_breaking_change_detector/buf.yaml b/tools/api_proto_breaking_change_detector/buf.yaml new file mode 100644 index 0000000000000..7626ff1c7c8b1 --- /dev/null +++ b/tools/api_proto_breaking_change_detector/buf.yaml @@ -0,0 +1,23 @@ +version: v1beta1 +# commenting these out for now until I figure out how to automate buf.lock +#deps: + # TODO: should these point to specific commit hashes? + # + +# - buf.build/beta/udpa +# - buf.build/beta/googleapis +# - buf.build/beta/opencensus +# - buf.build/beta/prometheus +# - buf.build/beta/opentelemetry +breaking: + use: + - FIELD_SAME_ONEOF + - FIELD_SAME_JSON_NAME + - FIELD_SAME_NAME + - FIELD_SAME_TYPE + - FIELD_SAME_LABEL + - FILE_SAME_PACKAGE + + # needed for allowing removal/reserving of fields + - FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED + - FIELD_NO_DELETE_UNLESS_NAME_RESERVED \ No newline at end of file diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py new file mode 100644 index 0000000000000..44859f00fea98 --- /dev/null +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -0,0 +1,129 @@ +from abc import ABC, abstractmethod +import tempfile +from rules_python.python.runfiles import runfiles +from tools.run_command import run_command +from shutil import copyfile +import re +from pathlib import Path +import os + + +# generic breaking change detector for protos, extended by a wrapper class for a breaking change detector +class ProtoBreakingChangeDetector(ABC): + # stateless + @staticmethod + def is_breaking(path_to_before, path_to_after): + pass + + @staticmethod + def lock_file_changed(path_to_before, path_to_after): + pass + + +class ChangeDetectorError(Exception): + pass + + +class ChangeDetectorInitializeError(ChangeDetectorError): + pass + + +BUF_LOCK_FILE = "tmp.json" + + +class BufWrapper(ProtoBreakingChangeDetector): + + @staticmethod + def _run_buf(path_to_before, path_to_after, additional_args=None): + if not Path(path_to_before).is_file(): + raise ValueError(f"path_to_before {path_to_before} does not exist") + + if not Path(path_to_after).is_file(): + raise ValueError(f"path_to_after {path_to_after} does not exist") + + # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) + # 2) copy buf.yaml into temp dir + # 3) copy start file into temp dir + # 4) buf build -o tmp_file + # 5) copy changed file into temp dir + # 6) buf breaking --against tmp_file + # 7) check for differences (if changes are breaking, there should be none) + + temp_dir = tempfile.TemporaryDirectory( + prefix=str(Path(".").absolute()) + + os.sep) # buf requires protobuf files to be in a subdirectory of cwd + buf_path = runfiles.Create().Rlocation("com_github_bufbuild_buf/bin/buf") + + buf_config_loc = Path(".", "tools", "api_proto_breaking_change_detector") + + #copyfile(Path(buf_config_loc, "buf.lock"), Path(".", "buf.lock")) # not needed? refer to comment below + yaml_file_loc = Path(".", "buf.yaml") + copyfile(Path(buf_config_loc, "buf.yaml"), yaml_file_loc) + + # TODO: figure out how to automatically pull buf deps + # `buf mod update` doesn't seem to do anything, and the first test will fail because it forces buf to automatically start downloading the deps + # bcode, bout, berr = run_command(f"{buf_path} mod update") + # bout, berr = ''.join(bout), ''.join(berr) + + target = Path(temp_dir.name, f"{Path(path_to_before).stem}.proto") + + buf_args = [ + "--path", + str(target.relative_to( + Path(".").absolute())), # buf requires relative pathing for roots... + "--config", + str(yaml_file_loc), + ] + if additional_args is not None: + buf_args.extend(additional_args) + + copyfile(path_to_before, target) + + initial_code, initial_out, initial_err = run_command( + ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) + initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) + + if len(initial_out) > 0 or len(initial_err) > 0: + raise ChangeDetectorInitializeError("Unexpected error during init") + + lock_location = Path(temp_dir.name, BUF_LOCK_FILE) + with open(lock_location) as f: + initial_lock = f.readlines() + + copyfile(path_to_after, target) + + final_code, final_out, final_err = run_command( + ' '.join( + [buf_path, f"breaking --against {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) + final_out, final_err = ''.join(final_out), ''.join(final_err) + + # new with buf: lock must be manually re-built + # but here we only re-build if there weren't any detected breaking changes + if len(final_out) == len(final_err) == 0: + _, _, _ = run_command( + ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) + with open(lock_location) as f: + final_lock = f.readlines() + + temp_dir.cleanup() + + return (initial_code, initial_out, + initial_err), (final_code, final_out, final_err), initial_lock, final_lock + + @staticmethod + def is_breaking(path_to_before, path_to_after, additional_args=None): + _, final_result, _, _ = BufWrapper._run_buf(path_to_before, path_to_after, additional_args) + + _, final_out, final_err = final_result + + # Ways buf output could be indicative of a breaking change: + # 1) stdout/stderr is nonempty + # 2) stdout/stderr contains "Failure" + break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) + return break_condition(final_out) or break_condition(final_err) + + @staticmethod + def lock_file_changed(path_to_before, path_to_after, additional_args=None): + _, _, initial_lock, final_lock = BufWrapper._run_buf( + path_to_before, path_to_after, additional_args) + return any(before != after for before, after in zip(initial_lock, final_lock)) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py new file mode 100644 index 0000000000000..3c4d13106f865 --- /dev/null +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -0,0 +1,103 @@ +from pathlib import Path +import unittest + +from detector import BufWrapper, ChangeDetectorInitializeError + + +class BufTests(unittest.TestCase): + + def run_buf_test(self, testname, is_breaking, expects_changes, additional_args=None): + tests_path = Path( + Path(__file__).absolute().parent.parent, "testdata", + "api_proto_breaking_change_detector", "breaking" if is_breaking else "allowed") + + current = Path(tests_path, f"{testname}_current") + changed = Path(tests_path, f"{testname}_next") + + breaking_response = BufWrapper.is_breaking(current, changed, additional_args) + self.assertEqual(breaking_response, is_breaking) + + lock_file_changed_response = BufWrapper.lock_file_changed(current, changed, additional_args) + self.assertEqual(lock_file_changed_response, expects_changes) + + +class TestBreakingChanges(BufTests): + + def test_change_field_id(self): + self.run_buf_test( + self.test_change_field_id.__name__, is_breaking=True, expects_changes=False) + + def test_change_field_type(self): + self.run_buf_test( + self.test_change_field_type.__name__, is_breaking=True, expects_changes=False) + + def test_change_field_plurality(self): + self.run_buf_test( + self.test_change_field_plurality.__name__, is_breaking=True, expects_changes=False) + + def test_change_field_name(self): + self.run_buf_test( + self.test_change_field_name.__name__, is_breaking=True, expects_changes=False) + + def test_change_package_name(self): + self.run_buf_test( + self.test_change_package_name.__name__, is_breaking=True, expects_changes=False) + + def test_change_field_from_oneof(self): + self.run_buf_test( + self.test_change_field_from_oneof.__name__, is_breaking=True, expects_changes=False) + + def test_change_field_to_oneof(self): + self.run_buf_test( + self.test_change_field_to_oneof.__name__, is_breaking=True, expects_changes=False) + + @unittest.skip("PGV field support not yet added to buf") + def test_change_pgv_field(self): + self.run_buf_test( + self.test_change_pgv_field.__name__, is_breaking=True, expects_changes=False) + + @unittest.skip("PGV message option support not yet added to buf") + def test_change_pgv_message(self): + self.run_buf_test( + self.test_change_pgv_message.__name__, is_breaking=True, expects_changes=False) + + @unittest.skip("PGV oneof option support not yet added to buf") + def test_change_pgv_oneof(self): + self.run_buf_test( + self.test_change_pgv_oneof.__name__, is_breaking=True, expects_changes=False) + + +class TestAllowedChanges(BufTests): + + def test_add_comment(self): + self.run_buf_test(self.test_add_comment.__name__, is_breaking=False, expects_changes=True) + + def test_add_field(self): + self.run_buf_test(self.test_add_field.__name__, is_breaking=False, expects_changes=True) + + def test_add_option(self): + self.run_buf_test(self.test_add_option.__name__, is_breaking=False, expects_changes=True) + + def test_add_enum_value(self): + self.run_buf_test( + self.test_add_enum_value.__name__, is_breaking=False, expects_changes=True) + + def test_remove_and_reserve_field(self): + self.run_buf_test( + self.test_remove_and_reserve_field.__name__, is_breaking=False, expects_changes=True) + + # copied from protolock tests but might remove + # It doesn't make sense to evaluate 'forcing' a breaking change in buf because by default, + # buf lets you re-build without checking for breaking changes + # Buf does not require forcing breaking changes into the lock file like protolock does + @unittest.skip("'forcing' a breaking change does not make sense for buf") + def test_force_breaking_change(self): + self.run_buf_test( + self.test_force_breaking_change.__name__, + is_breaking=False, + expects_changes=True, + additional_args=["--force"]) + + +if __name__ == '__main__': + unittest.main() diff --git a/tools/testdata/api_proto_breaking_change_detector/BUILD b/tools/testdata/api_proto_breaking_change_detector/BUILD new file mode 100644 index 0000000000000..e9e797dca8c6f --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/BUILD @@ -0,0 +1,36 @@ +licenses(["notice"]) # Apache 2 + +exports_files([ + "allowed/test_add_comment_current", + "allowed/test_add_enum_value_next", + "allowed/test_add_option_current", + "allowed/test_force_breaking_change_next", + "allowed/test_add_comment_next", + "allowed/test_add_field_current", + "allowed/test_add_option_next", + "allowed/test_remove_and_reserve_field_current", + "allowed/test_add_enum_value_current", + "allowed/test_add_field_next", + "allowed/test_force_breaking_change_current", + "allowed/test_remove_and_reserve_field_next", + "breaking/test_change_field_from_oneof_current", + "breaking/test_change_field_name_next", + "breaking/test_change_field_type_current", + "breaking/test_change_pgv_field_next", + "breaking/test_change_field_from_oneof_next", + "breaking/test_change_field_plurality_current", + "breaking/test_change_field_type_next", + "breaking/test_change_pgv_message_current", + "breaking/test_change_field_id_current", + "breaking/test_change_field_plurality_next", + "breaking/test_change_package_name_current", + "breaking/test_change_pgv_message_next", + "breaking/test_change_field_id_next", + "breaking/test_change_field_to_oneof_current", + "breaking/test_change_package_name_next", + "breaking/test_change_pgv_oneof_current", + "breaking/test_change_field_name_current", + "breaking/test_change_field_to_oneof_next", + "breaking/test_change_pgv_field_current", + "breaking/test_change_pgv_oneof_next", +]) diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current new file mode 100644 index 0000000000000..819ad5b8b9e22 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current @@ -0,0 +1,9 @@ +syntax = "proto3"; +package test.protos.breaking; + +option java_package = "io.envoyproxy.envoy.protolock"; +option java_outer_classname = "EnvoyProtolock"; +option java_multiple_files = true; + +message CommonExtensionConfig { +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next new file mode 100644 index 0000000000000..422405f5538b0 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +option java_package = "io.envoyproxy.envoy.protolock"; +option java_outer_classname = "EnvoyProtolock"; +option java_multiple_files = true; + +// Common configuration for all tap extensions. +message CommonExtensionConfig { +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current new file mode 100644 index 0000000000000..89e9818cdd29f --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current @@ -0,0 +1,17 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SearchRequest { + string query = 1; + int32 page_number = 2; + int32 result_per_page = 3; + enum Corpus { + UNIVERSAL = 0; + WEB = 1; + IMAGES = 2; + LOCAL = 3; + NEWS = 4; + PRODUCTS = 5; + } + Corpus corpus = 4; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next new file mode 100644 index 0000000000000..53aac251a92dc --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next @@ -0,0 +1,18 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SearchRequest { + string query = 1; + int32 page_number = 2; + int32 result_per_page = 3; + enum Corpus { + UNIVERSAL = 0; + WEB = 1; + IMAGES = 2; + LOCAL = 3; + NEWS = 4; + PRODUCTS = 5; + VIDEO = 6; + } + Corpus corpus = 4; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current new file mode 100644 index 0000000000000..0830a96e127c5 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 3; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next new file mode 100644 index 0000000000000..af859a0cc5fbb --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next @@ -0,0 +1,7 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 3; + float four = 4; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current new file mode 100644 index 0000000000000..2f0892d5d91b2 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current @@ -0,0 +1,9 @@ +syntax = "proto3"; +package test.protos.breaking; + +option java_package = "io.envoyproxy.envoy.protolock"; +option java_outer_classname = "EnvoyProtolock"; + +// Common configuration for all tap extensions. +message CommonExtensionConfig { +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next new file mode 100644 index 0000000000000..422405f5538b0 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +option java_package = "io.envoyproxy.envoy.protolock"; +option java_outer_classname = "EnvoyProtolock"; +option java_multiple_files = true; + +// Common configuration for all tap extensions. +message CommonExtensionConfig { +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current new file mode 100644 index 0000000000000..0830a96e127c5 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 3; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next new file mode 100644 index 0000000000000..89de3ac218470 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + float three = 3; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current new file mode 100644 index 0000000000000..af859a0cc5fbb --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current @@ -0,0 +1,7 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 3; + float four = 4; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next new file mode 100644 index 0000000000000..2607958e03119 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next @@ -0,0 +1,9 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + reserved 4; + reserved "four"; + + string three = 3; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current new file mode 100644 index 0000000000000..0a1e8f1064229 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + string name = 4; + string sub_message = 9; + float special = 10; + } +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next new file mode 100644 index 0000000000000..4e1ddfb1b47d4 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + string name = 4; + string sub_message = 9; + } + float special = 10; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current new file mode 100644 index 0000000000000..0830a96e127c5 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 3; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next new file mode 100644 index 0000000000000..62babdd42530b --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string three = 42; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current new file mode 100644 index 0000000000000..0645d218049c3 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string seventeen = 17; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next new file mode 100644 index 0000000000000..669225e3ee531 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string twenty = 17; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current new file mode 100644 index 0000000000000..e1321b707cc2f --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string from = 1; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next new file mode 100644 index 0000000000000..2817267b2f292 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + repeated string from = 1; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current new file mode 100644 index 0000000000000..5d1debb193775 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + string name = 4; + string sub_message = 9; + } + float loner = 10; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next new file mode 100644 index 0000000000000..b61743cef7e48 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + string name = 4; + string sub_message = 9; + float loner = 10; + } +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current new file mode 100644 index 0000000000000..f1cff9ac4483d --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + float pi = 314; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next new file mode 100644 index 0000000000000..fe53195eef988 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string pi = 314; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current new file mode 100644 index 0000000000000..6f2260ac544e8 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string passcode = 69; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next new file mode 100644 index 0000000000000..da064e9bdfacf --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package wrong.package; + +message SampleMessage { + string passcode = 69; +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_current new file mode 100644 index 0000000000000..72b792e819e5e --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_current @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string useremail = 1 [(validate.rules).string.email = true]; +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_next new file mode 100644 index 0000000000000..2715676735f30 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_field_next @@ -0,0 +1,6 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + string useremail = 1 [(validate.rules).string.email = false]; +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_current new file mode 100644 index 0000000000000..1df9b09c14b28 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_current @@ -0,0 +1,8 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + option (validate.disabled) = true; + + SampleMessage q = 543; +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_next new file mode 100644 index 0000000000000..62fd2266f00fc --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_message_next @@ -0,0 +1,8 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + option (validate.disabled) = false; + + SampleMessage q = 543; +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current new file mode 100644 index 0000000000000..47f17f9ec0228 --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + option (validate.required) = true; + string name = 4; + string sub_message = 9;; + } +} \ No newline at end of file diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next new file mode 100644 index 0000000000000..1fd45b1e2b58e --- /dev/null +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next @@ -0,0 +1,10 @@ +syntax = "proto3"; +package test.protos.breaking; + +message SampleMessage { + oneof test_oneof { + option (validate.required) = false; + string name = 4; + string sub_message = 9;; + } +} \ No newline at end of file From 8574b7e92bcdff738474716502d6708c1a2a35d5 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 28 Jul 2021 18:32:05 +0000 Subject: [PATCH 02/16] check exit code too Signed-off-by: Yaseen Alkhafaji --- .../api_proto_breaking_change_detector/detector.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 44859f00fea98..a497fa93c835e 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -83,7 +83,7 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) - if len(initial_out) > 0 or len(initial_err) > 0: + if initial_code != 0 or len(initial_out) > 0 or len(initial_err) > 0: raise ChangeDetectorInitializeError("Unexpected error during init") lock_location = Path(temp_dir.name, BUF_LOCK_FILE) @@ -99,7 +99,7 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): # new with buf: lock must be manually re-built # but here we only re-build if there weren't any detected breaking changes - if len(final_out) == len(final_err) == 0: + if len(final_out) == len(final_err) == final_code == 0: _, _, _ = run_command( ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) with open(lock_location) as f: @@ -114,13 +114,14 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): def is_breaking(path_to_before, path_to_after, additional_args=None): _, final_result, _, _ = BufWrapper._run_buf(path_to_before, path_to_after, additional_args) - _, final_out, final_err = final_result + final_code, final_out, final_err = final_result # Ways buf output could be indicative of a breaking change: - # 1) stdout/stderr is nonempty - # 2) stdout/stderr contains "Failure" + # 1) run_command code is not 0 (e.g. it's 100) + # 2) stdout/stderr is nonempty + # 3) stdout/stderr contains "Failure" break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) - return break_condition(final_out) or break_condition(final_err) + return final_code != 0 or break_condition(final_out) or break_condition(final_err) @staticmethod def lock_file_changed(path_to_before, path_to_after, additional_args=None): From 29e7dd2a431b12ed7d6b25c55733e5caa867443a Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 28 Jul 2021 19:16:36 +0000 Subject: [PATCH 03/16] small test cleanup Signed-off-by: Yaseen Alkhafaji --- .../detector_test.py | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index 3c4d13106f865..33c2ebec80a24 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -23,68 +23,62 @@ def run_buf_test(self, testname, is_breaking, expects_changes, additional_args=N class TestBreakingChanges(BufTests): + def run_breaking_test(self, testname): + self.run_buf_test(testname, is_breaking=True, expects_changes=False) + def test_change_field_id(self): - self.run_buf_test( - self.test_change_field_id.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_id.__name__) def test_change_field_type(self): - self.run_buf_test( - self.test_change_field_type.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_type.__name__) def test_change_field_plurality(self): - self.run_buf_test( - self.test_change_field_plurality.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_plurality.__name__) def test_change_field_name(self): - self.run_buf_test( - self.test_change_field_name.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_name.__name__) def test_change_package_name(self): - self.run_buf_test( - self.test_change_package_name.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_package_name.__name__) def test_change_field_from_oneof(self): - self.run_buf_test( - self.test_change_field_from_oneof.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_from_oneof.__name__) def test_change_field_to_oneof(self): - self.run_buf_test( - self.test_change_field_to_oneof.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_field_to_oneof.__name__) @unittest.skip("PGV field support not yet added to buf") def test_change_pgv_field(self): - self.run_buf_test( - self.test_change_pgv_field.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_pgv_field.__name__) @unittest.skip("PGV message option support not yet added to buf") def test_change_pgv_message(self): - self.run_buf_test( - self.test_change_pgv_message.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_pgv_message.__name__) @unittest.skip("PGV oneof option support not yet added to buf") def test_change_pgv_oneof(self): - self.run_buf_test( - self.test_change_pgv_oneof.__name__, is_breaking=True, expects_changes=False) + self.run_breaking_test(self.test_change_pgv_oneof.__name__) class TestAllowedChanges(BufTests): + def run_allowed_test(self, testname, additional_args=None): + self.run_buf_test(testname, is_breaking=False, expects_changes=True) + def test_add_comment(self): - self.run_buf_test(self.test_add_comment.__name__, is_breaking=False, expects_changes=True) + self.run_allowed_test(self.test_add_comment.__name__) def test_add_field(self): - self.run_buf_test(self.test_add_field.__name__, is_breaking=False, expects_changes=True) + self.run_allowed_test(self.test_add_field.__name__) def test_add_option(self): - self.run_buf_test(self.test_add_option.__name__, is_breaking=False, expects_changes=True) + self.run_allowed_test(self.test_add_option.__name__) def test_add_enum_value(self): - self.run_buf_test( - self.test_add_enum_value.__name__, is_breaking=False, expects_changes=True) + self.run_allowed_test(self.test_add_enum_value.__name__) def test_remove_and_reserve_field(self): - self.run_buf_test( - self.test_remove_and_reserve_field.__name__, is_breaking=False, expects_changes=True) + self.run_allowed_test(self.test_remove_and_reserve_field.__name__) # copied from protolock tests but might remove # It doesn't make sense to evaluate 'forcing' a breaking change in buf because by default, @@ -92,11 +86,7 @@ def test_remove_and_reserve_field(self): # Buf does not require forcing breaking changes into the lock file like protolock does @unittest.skip("'forcing' a breaking change does not make sense for buf") def test_force_breaking_change(self): - self.run_buf_test( - self.test_force_breaking_change.__name__, - is_breaking=False, - expects_changes=True, - additional_args=["--force"]) + self.run_allowed_test(self.test_force_breaking_change.__name__, additional_args=["--force"]) if __name__ == '__main__': From 2f9fdef7ffee096a1678d3001aacd865ee071e8b Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 28 Jul 2021 21:08:31 +0000 Subject: [PATCH 04/16] make tests detector-independent Signed-off-by: Yaseen Alkhafaji --- .../detector_test.py | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index 33c2ebec80a24..f4945e56ac6e8 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -4,9 +4,10 @@ from detector import BufWrapper, ChangeDetectorInitializeError -class BufTests(unittest.TestCase): +class BreakingChangeDetectorTests(object): + detector_type = None - def run_buf_test(self, testname, is_breaking, expects_changes, additional_args=None): + def run_detector_test(self, testname, is_breaking, expects_changes, additional_args=None): tests_path = Path( Path(__file__).absolute().parent.parent, "testdata", "api_proto_breaking_change_detector", "breaking" if is_breaking else "allowed") @@ -14,17 +15,18 @@ def run_buf_test(self, testname, is_breaking, expects_changes, additional_args=N current = Path(tests_path, f"{testname}_current") changed = Path(tests_path, f"{testname}_next") - breaking_response = BufWrapper.is_breaking(current, changed, additional_args) + breaking_response = self.detector_type.is_breaking(current, changed, additional_args) self.assertEqual(breaking_response, is_breaking) - lock_file_changed_response = BufWrapper.lock_file_changed(current, changed, additional_args) + lock_file_changed_response = self.detector_type.lock_file_changed( + current, changed, additional_args) self.assertEqual(lock_file_changed_response, expects_changes) -class TestBreakingChanges(BufTests): +class TestBreakingChanges(BreakingChangeDetectorTests): def run_breaking_test(self, testname): - self.run_buf_test(testname, is_breaking=True, expects_changes=False) + self.run_detector_test(testname, is_breaking=True, expects_changes=False) def test_change_field_id(self): self.run_breaking_test(self.test_change_field_id.__name__) @@ -47,23 +49,20 @@ def test_change_field_from_oneof(self): def test_change_field_to_oneof(self): self.run_breaking_test(self.test_change_field_to_oneof.__name__) - @unittest.skip("PGV field support not yet added to buf") def test_change_pgv_field(self): self.run_breaking_test(self.test_change_pgv_field.__name__) - @unittest.skip("PGV message option support not yet added to buf") def test_change_pgv_message(self): self.run_breaking_test(self.test_change_pgv_message.__name__) - @unittest.skip("PGV oneof option support not yet added to buf") def test_change_pgv_oneof(self): self.run_breaking_test(self.test_change_pgv_oneof.__name__) -class TestAllowedChanges(BufTests): +class TestAllowedChanges(BreakingChangeDetectorTests): def run_allowed_test(self, testname, additional_args=None): - self.run_buf_test(testname, is_breaking=False, expects_changes=True) + self.run_detector_test(testname, is_breaking=False, expects_changes=True) def test_add_comment(self): self.run_allowed_test(self.test_add_comment.__name__) @@ -80,13 +79,32 @@ def test_add_enum_value(self): def test_remove_and_reserve_field(self): self.run_allowed_test(self.test_remove_and_reserve_field.__name__) + def test_force_breaking_change(self): + self.run_allowed_test(self.test_force_breaking_change.__name__, additional_args=["--force"]) + + +class BufTests(TestAllowedChanges, TestBreakingChanges, unittest.TestCase): + detector_type = BufWrapper + + @unittest.skip("PGV field support not yet added to buf") + def test_change_pgv_field(self): + pass + + @unittest.skip("PGV message option support not yet added to buf") + def test_change_pgv_message(self): + pass + + @unittest.skip("PGV oneof option support not yet added to buf") + def test_change_pgv_oneof(self): + pass + # copied from protolock tests but might remove # It doesn't make sense to evaluate 'forcing' a breaking change in buf because by default, # buf lets you re-build without checking for breaking changes # Buf does not require forcing breaking changes into the lock file like protolock does @unittest.skip("'forcing' a breaking change does not make sense for buf") def test_force_breaking_change(self): - self.run_allowed_test(self.test_force_breaking_change.__name__, additional_args=["--force"]) + pass if __name__ == '__main__': From cf0173a165cc60dd02e813edc83459ed7fc3ef41 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 28 Jul 2021 21:23:04 +0000 Subject: [PATCH 05/16] add comment describing tool behavior Signed-off-by: Yaseen Alkhafaji --- tools/api_proto_breaking_change_detector/detector.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index a497fa93c835e..1c97c7e7bee30 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -6,6 +6,17 @@ import re from pathlib import Path import os +""" +This tool is used to detect "breaking changes" in protobuf files, to ensure proper backwards-compatibility in protobuf API updates. +The tool can check for breaking changes of a single API by taking 2 .proto file paths as input (before and after) and outputting a bool `is_breaking`. + +The breaking change detector creates a temporary directory, copies in each file to compute a protobuf "state", +computes a diff of the "before" and "after" states, and runs the diff against a set of rules to determine if there was a breaking change. + +The tool is currently implemented with buf (https://buf.build/) + +In future iterations the tool will support the comparison of multiple files, by taking git commits as input and evaluating the diff against the ruleset +""" # generic breaking change detector for protos, extended by a wrapper class for a breaking change detector From 22efce436ec87a3b88d47890bc65438ee2ee1762 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Thu, 29 Jul 2021 23:04:08 +0000 Subject: [PATCH 06/16] make detector class stateful Signed-off-by: Yaseen Alkhafaji --- .../detector.py | 58 ++++++++++--------- .../detector_test.py | 8 ++- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 1c97c7e7bee30..aaab803a7955f 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -14,20 +14,26 @@ computes a diff of the "before" and "after" states, and runs the diff against a set of rules to determine if there was a breaking change. The tool is currently implemented with buf (https://buf.build/) - -In future iterations the tool will support the comparison of multiple files, by taking git commits as input and evaluating the diff against the ruleset """ # generic breaking change detector for protos, extended by a wrapper class for a breaking change detector class ProtoBreakingChangeDetector(ABC): - # stateless - @staticmethod - def is_breaking(path_to_before, path_to_after): + + @abstractmethod + def __init__(self, path_to_before, path_to_after, additional_args=None): + pass + + @abstractmethod + def run_detector(self): pass - @staticmethod - def lock_file_changed(path_to_before, path_to_after): + @abstractmethod + def is_breaking(self): + pass + + @abstractmethod + def lock_file_changed(self): pass @@ -44,14 +50,18 @@ class ChangeDetectorInitializeError(ChangeDetectorError): class BufWrapper(ProtoBreakingChangeDetector): - @staticmethod - def _run_buf(path_to_before, path_to_after, additional_args=None): + def __init__(self, path_to_before, path_to_after, additional_args=None): if not Path(path_to_before).is_file(): raise ValueError(f"path_to_before {path_to_before} does not exist") if not Path(path_to_after).is_file(): raise ValueError(f"path_to_after {path_to_after} does not exist") + self.path_to_before = path_to_before + self.path_to_after = path_to_after + self.additional_args = additional_args + + def run_detector(self): # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) # 2) copy buf.yaml into temp dir # 3) copy start file into temp dir @@ -76,7 +86,7 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): # bcode, bout, berr = run_command(f"{buf_path} mod update") # bout, berr = ''.join(bout), ''.join(berr) - target = Path(temp_dir.name, f"{Path(path_to_before).stem}.proto") + target = Path(temp_dir.name, f"{Path(self.path_to_before).stem}.proto") buf_args = [ "--path", @@ -85,10 +95,10 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): "--config", str(yaml_file_loc), ] - if additional_args is not None: - buf_args.extend(additional_args) + if self.additional_args is not None: + buf_args.extend(self.additional_args) - copyfile(path_to_before, target) + copyfile(self.path_to_before, target) initial_code, initial_out, initial_err = run_command( ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) @@ -101,7 +111,7 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): with open(lock_location) as f: initial_lock = f.readlines() - copyfile(path_to_after, target) + copyfile(self.path_to_after, target) final_code, final_out, final_err = run_command( ' '.join( @@ -118,14 +128,13 @@ def _run_buf(path_to_before, path_to_after, additional_args=None): temp_dir.cleanup() - return (initial_code, initial_out, - initial_err), (final_code, final_out, final_err), initial_lock, final_lock - - @staticmethod - def is_breaking(path_to_before, path_to_after, additional_args=None): - _, final_result, _, _ = BufWrapper._run_buf(path_to_before, path_to_after, additional_args) + self.initial_result = initial_code, initial_out, initial_err + self.final_result = final_code, final_out, final_err + self.initial_lock = initial_lock + self.final_lock = final_lock - final_code, final_out, final_err = final_result + def is_breaking(self): + final_code, final_out, final_err = self.final_result # Ways buf output could be indicative of a breaking change: # 1) run_command code is not 0 (e.g. it's 100) @@ -134,8 +143,5 @@ def is_breaking(path_to_before, path_to_after, additional_args=None): break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) return final_code != 0 or break_condition(final_out) or break_condition(final_err) - @staticmethod - def lock_file_changed(path_to_before, path_to_after, additional_args=None): - _, _, initial_lock, final_lock = BufWrapper._run_buf( - path_to_before, path_to_after, additional_args) - return any(before != after for before, after in zip(initial_lock, final_lock)) + def lock_file_changed(self): + return any(before != after for before, after in zip(self.initial_lock, self.final_lock)) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index f4945e56ac6e8..71c0ffce2e8c2 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -15,11 +15,13 @@ def run_detector_test(self, testname, is_breaking, expects_changes, additional_a current = Path(tests_path, f"{testname}_current") changed = Path(tests_path, f"{testname}_next") - breaking_response = self.detector_type.is_breaking(current, changed, additional_args) + detector_obj = self.detector_type(current, changed, additional_args) + detector_obj.run_detector() + + breaking_response = detector_obj.is_breaking() self.assertEqual(breaking_response, is_breaking) - lock_file_changed_response = self.detector_type.lock_file_changed( - current, changed, additional_args) + lock_file_changed_response = detector_obj.lock_file_changed() self.assertEqual(lock_file_changed_response, expects_changes) From f402d6133990e41edb0b21b4819060a6d9013f85 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Thu, 29 Jul 2021 23:08:58 +0000 Subject: [PATCH 07/16] comment on test suite Signed-off-by: Yaseen Alkhafaji --- tools/api_proto_breaking_change_detector/detector_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index 71c0ffce2e8c2..6bbe29d896d96 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -2,6 +2,11 @@ import unittest from detector import BufWrapper, ChangeDetectorInitializeError +""" +Tests breaking change detectors (e.g. buf) against different proto changes to ensure proper behavior in +`allowed` and `breaking` circumstances. Although the dependency likely already tests for these circumstances, +these specify Envoy's requirements and ensure that tool behavior is consistent across dependency updates. +""" class BreakingChangeDetectorTests(object): From 21f14562fee3d18d1143fe537499046cc76c2d11 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Thu, 29 Jul 2021 23:39:55 +0000 Subject: [PATCH 08/16] move buf bazel repo to api/bazel Signed-off-by: Yaseen Alkhafaji --- api/bazel/repositories.bzl | 19 +++++++++++++++++++ api/bazel/repository_locations.bzl | 12 ++++++++++++ bazel/external/api_buf.BUILD | 11 ----------- bazel/repositories.bzl | 9 --------- bazel/repository_locations.bzl | 12 ------------ generated_api_shadow/bazel/repositories.bzl | 19 +++++++++++++++++++ .../bazel/repository_locations.bzl | 12 ++++++++++++ 7 files changed, 62 insertions(+), 32 deletions(-) delete mode 100644 bazel/external/api_buf.BUILD diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 74e19f831179f..5f8cca8fcd0a3 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -47,6 +47,11 @@ def api_dependencies(): name = "opentelemetry_proto", build_file_content = OPENTELEMETRY_LOGS_BUILD_CONTENT, ) + external_http_archive( + name = "com_github_bufbuild_buf", + build_file_content = BUF_BUILD_CONTENT, + tags = ["manual"], + ) PROMETHEUSMETRICS_BUILD_CONTENT = """ load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library") @@ -150,3 +155,17 @@ go_proto_library( visibility = ["//visibility:public"], ) """ + +BUF_BUILD_CONTENT = """ +package( + default_visibility = ["//visibility:public"], +) + +filegroup( + name = "buf", + srcs = [ + "@com_github_bufbuild_buf//:bin/buf", + ], + tags = ["manual"], +) +""" diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 968c6a9ffa286..a2284503f8692 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -118,4 +118,16 @@ REPOSITORY_LOCATIONS_SPEC = dict( urls = ["https://github.com/open-telemetry/opentelemetry-proto/archive/v{version}.tar.gz"], use_category = ["api"], ), + com_github_bufbuild_buf = dict( + project_name = "buf", + project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs + project_url = "https://buf.build", + version = "0.46.0", + sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", + strip_prefix = "buf", + urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], + release_date = "2021-07-27", + use_category = ["api"], + tags = ["manual"], + ), ) diff --git a/bazel/external/api_buf.BUILD b/bazel/external/api_buf.BUILD deleted file mode 100644 index dfab37624106c..0000000000000 --- a/bazel/external/api_buf.BUILD +++ /dev/null @@ -1,11 +0,0 @@ -package( - default_visibility = ["//visibility:public"], -) - -filegroup( - name = "buf", - srcs = [ - "@com_github_bufbuild_buf//:bin/buf", - ], - tags = ["manual"], -) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 87d7ef7b30bcc..be758fa4cebfd 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -192,8 +192,6 @@ def envoy_dependencies(skip_targets = []): _com_github_wasmtime() _com_github_wasm_c_api() - _com_github_bufbuild_buf() - switched_rules_by_language( name = "com_google_googleapis_imports", cc = True, @@ -1026,13 +1024,6 @@ filegroup( build_file_content = BUILD_ALL_CONTENT, ) -def _com_github_bufbuild_buf(): - external_http_archive( - name = "com_github_bufbuild_buf", - build_file = "@envoy//bazel/external:api_buf.BUILD", - tags = ["manual"], - ) - def _foreign_cc_dependencies(): external_http_archive("rules_foreign_cc") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index c54318cfd9451..2b2b53eba6688 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1072,16 +1072,4 @@ REPOSITORY_LOCATIONS_SPEC = dict( release_date = "2018-12-18", cpe = "N/A", ), - com_github_bufbuild_buf = dict( - project_name = "buf", - project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs - project_url = "https://buf.build", - version = "0.46.0", - sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", - strip_prefix = "buf", - urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], - release_date = "2021-07-27", - use_category = ["api"], - tags = ["manual"], - ), ) diff --git a/generated_api_shadow/bazel/repositories.bzl b/generated_api_shadow/bazel/repositories.bzl index 74e19f831179f..5f8cca8fcd0a3 100644 --- a/generated_api_shadow/bazel/repositories.bzl +++ b/generated_api_shadow/bazel/repositories.bzl @@ -47,6 +47,11 @@ def api_dependencies(): name = "opentelemetry_proto", build_file_content = OPENTELEMETRY_LOGS_BUILD_CONTENT, ) + external_http_archive( + name = "com_github_bufbuild_buf", + build_file_content = BUF_BUILD_CONTENT, + tags = ["manual"], + ) PROMETHEUSMETRICS_BUILD_CONTENT = """ load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library") @@ -150,3 +155,17 @@ go_proto_library( visibility = ["//visibility:public"], ) """ + +BUF_BUILD_CONTENT = """ +package( + default_visibility = ["//visibility:public"], +) + +filegroup( + name = "buf", + srcs = [ + "@com_github_bufbuild_buf//:bin/buf", + ], + tags = ["manual"], +) +""" diff --git a/generated_api_shadow/bazel/repository_locations.bzl b/generated_api_shadow/bazel/repository_locations.bzl index 968c6a9ffa286..a2284503f8692 100644 --- a/generated_api_shadow/bazel/repository_locations.bzl +++ b/generated_api_shadow/bazel/repository_locations.bzl @@ -118,4 +118,16 @@ REPOSITORY_LOCATIONS_SPEC = dict( urls = ["https://github.com/open-telemetry/opentelemetry-proto/archive/v{version}.tar.gz"], use_category = ["api"], ), + com_github_bufbuild_buf = dict( + project_name = "buf", + project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs + project_url = "https://buf.build", + version = "0.46.0", + sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", + strip_prefix = "buf", + urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], + release_date = "2021-07-27", + use_category = ["api"], + tags = ["manual"], + ), ) From ff59f6fde24091bcf4b9a749360a31bdbaadb482 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Fri, 30 Jul 2021 22:23:40 +0000 Subject: [PATCH 09/16] fix nits from second pass Signed-off-by: Yaseen Alkhafaji --- .../detector.py | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index aaab803a7955f..d72e0640519e2 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -45,7 +45,7 @@ class ChangeDetectorInitializeError(ChangeDetectorError): pass -BUF_LOCK_FILE = "tmp.json" +BUF_STATE_FILE = "tmp.json" class BufWrapper(ProtoBreakingChangeDetector): @@ -57,9 +57,9 @@ def __init__(self, path_to_before, path_to_after, additional_args=None): if not Path(path_to_after).is_file(): raise ValueError(f"path_to_after {path_to_after} does not exist") - self.path_to_before = path_to_before - self.path_to_after = path_to_after - self.additional_args = additional_args + self._path_to_before = path_to_before + self._path_to_after = path_to_after + self._additional_args = additional_args def run_detector(self): # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) @@ -86,7 +86,7 @@ def run_detector(self): # bcode, bout, berr = run_command(f"{buf_path} mod update") # bout, berr = ''.join(bout), ''.join(berr) - target = Path(temp_dir.name, f"{Path(self.path_to_before).stem}.proto") + target = Path(temp_dir.name, f"{Path(self._path_to_before).stem}.proto") buf_args = [ "--path", @@ -95,35 +95,37 @@ def run_detector(self): "--config", str(yaml_file_loc), ] - if self.additional_args is not None: - buf_args.extend(self.additional_args) + if self._additional_args is not None: + buf_args.extend(self._additional_args) - copyfile(self.path_to_before, target) + copyfile(self._path_to_before, target) initial_code, initial_out, initial_err = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) + ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) if initial_code != 0 or len(initial_out) > 0 or len(initial_err) > 0: - raise ChangeDetectorInitializeError("Unexpected error during init") + raise ChangeDetectorInitializeError( + f"Unexpected error during init:\n\tExit Status Code: {initial_code}\n\tstdout: {initial_out}\n\t stderr: {initial_err}\n" + ) - lock_location = Path(temp_dir.name, BUF_LOCK_FILE) - with open(lock_location) as f: + lock_location = Path(temp_dir.name, BUF_STATE_FILE) + with open(lock_location, "r") as f: initial_lock = f.readlines() - copyfile(self.path_to_after, target) + copyfile(self._path_to_after, target) final_code, final_out, final_err = run_command( ' '.join( - [buf_path, f"breaking --against {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) + [buf_path, f"breaking --against {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) final_out, final_err = ''.join(final_out), ''.join(final_err) # new with buf: lock must be manually re-built # but here we only re-build if there weren't any detected breaking changes if len(final_out) == len(final_err) == final_code == 0: _, _, _ = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_LOCK_FILE)}", *buf_args])) - with open(lock_location) as f: + ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) + with open(lock_location, "r") as f: final_lock = f.readlines() temp_dir.cleanup() @@ -137,7 +139,7 @@ def is_breaking(self): final_code, final_out, final_err = self.final_result # Ways buf output could be indicative of a breaking change: - # 1) run_command code is not 0 (e.g. it's 100) + # 1) final_code (exit status code) is not 0 (e.g. it's 100) # 2) stdout/stderr is nonempty # 3) stdout/stderr contains "Failure" break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) From ec1b3de39334858f6d99464d43d3f9a71287b2f7 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Fri, 30 Jul 2021 22:51:25 +0000 Subject: [PATCH 10/16] add type hints to ProtoBreakingChangeDetector api Signed-off-by: Yaseen Alkhafaji --- .../detector.py | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index d72e0640519e2..3dfec2ddad1b5 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -6,6 +6,7 @@ import re from pathlib import Path import os +from typing import List """ This tool is used to detect "breaking changes" in protobuf files, to ensure proper backwards-compatibility in protobuf API updates. The tool can check for breaking changes of a single API by taking 2 .proto file paths as input (before and after) and outputting a bool `is_breaking`. @@ -21,19 +22,23 @@ class ProtoBreakingChangeDetector(ABC): @abstractmethod - def __init__(self, path_to_before, path_to_after, additional_args=None): + def __init__( + self, + path_to_before: str, + path_to_after: str, + additional_args: List[str] = None) -> None: pass @abstractmethod - def run_detector(self): + def run_detector(self) -> None: pass @abstractmethod - def is_breaking(self): + def is_breaking(self) -> bool: pass @abstractmethod - def lock_file_changed(self): + def lock_file_changed(self) -> bool: pass @@ -50,7 +55,11 @@ class ChangeDetectorInitializeError(ChangeDetectorError): class BufWrapper(ProtoBreakingChangeDetector): - def __init__(self, path_to_before, path_to_after, additional_args=None): + def __init__( + self, + path_to_before: str, + path_to_after: str, + additional_args: List[str] = None) -> None: if not Path(path_to_before).is_file(): raise ValueError(f"path_to_before {path_to_before} does not exist") @@ -61,7 +70,7 @@ def __init__(self, path_to_before, path_to_after, additional_args=None): self._path_to_after = path_to_after self._additional_args = additional_args - def run_detector(self): + def run_detector(self) -> None: # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) # 2) copy buf.yaml into temp dir # 3) copy start file into temp dir @@ -135,7 +144,7 @@ def run_detector(self): self.initial_lock = initial_lock self.final_lock = final_lock - def is_breaking(self): + def is_breaking(self) -> bool: final_code, final_out, final_err = self.final_result # Ways buf output could be indicative of a breaking change: @@ -145,5 +154,5 @@ def is_breaking(self): break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) return final_code != 0 or break_condition(final_out) or break_condition(final_err) - def lock_file_changed(self): + def lock_file_changed(self) -> bool: return any(before != after for before, after in zip(self.initial_lock, self.final_lock)) From 5640665131cc97c5edb67ba71795151eb86d6d20 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Fri, 30 Jul 2021 22:54:25 +0000 Subject: [PATCH 11/16] document ProtoBreakingChangeDetector api funcs Signed-off-by: Yaseen Alkhafaji --- .../detector.py | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 3dfec2ddad1b5..85ba797146ecb 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -27,18 +27,51 @@ def __init__( path_to_before: str, path_to_after: str, additional_args: List[str] = None) -> None: + """Initializes the breaking change detector, setting up any necessary config without actually running + the detector against any proto files. + + Takes in a single protobuf as 2 files, in a ``before`` state and an ``after`` state, + and checks if the ``after`` state violates any breaking change rules. + + :param path_to_before: absolute path to the .proto file in the before state + :type path_to_before: ``str`` + + :param path_to_after: absolute path to the .proto file in the after state + :type path_to_after: ``str`` + + :param additional_args: additional arguments for the breaking change detector CLI. May be tool-dependent. + Additional arguments are passed in both the "initialize" and "detect changes" steps. + :type additional_args: ``List[str]`` + """ pass @abstractmethod def run_detector(self) -> None: + """Runs the breaking change detector with the parameters given in ``__init__``. This method should populate + the detector's internal data such that `is_breaking` and `lock_file_changed` do not require any additional + invocations to the breaking change detector. + """ pass @abstractmethod def is_breaking(self) -> bool: + """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` + violate any breaking change rules specified by this breaking change detector. + + :return: a boolean flag indicating if the changes between ``before`` and ``after`` are breaking + :rtype: ``bool`` + """ pass @abstractmethod def lock_file_changed(self) -> bool: + """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` + cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change + detector is checking all of the protobuf features we are interested in. + + :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file + :rtype: ``bool`` + """ pass @@ -60,6 +93,22 @@ def __init__( path_to_before: str, path_to_after: str, additional_args: List[str] = None) -> None: + """Initializes the breaking change detector, setting up any necessary config without actually running + the detector against any proto files. + + Takes in a single protobuf as 2 files, in a ``before`` state and an ``after`` state, + and checks if the ``after`` state violates any breaking change rules. + + :param path_to_before: absolute path to the .proto file in the before state + :type path_to_before: ``str`` + + :param path_to_after: absolute path to the .proto file in the after state + :type path_to_after: ``str`` + + :param additional_args: additional arguments for the breaking change detector CLI. May be tool-dependent. + Additional arguments are passed in both the "initialize" and "detect changes" steps. + :type additional_args: ``List[str]`` + """ if not Path(path_to_before).is_file(): raise ValueError(f"path_to_before {path_to_before} does not exist") @@ -71,6 +120,10 @@ def __init__( self._additional_args = additional_args def run_detector(self) -> None: + """Runs the breaking change detector with the parameters given in ``__init__``. This method should populate + the detector's internal data such that `is_breaking` and `lock_file_changed` do not require any additional + invocations to the breaking change detector. + """ # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) # 2) copy buf.yaml into temp dir # 3) copy start file into temp dir @@ -94,7 +147,6 @@ def run_detector(self) -> None: # `buf mod update` doesn't seem to do anything, and the first test will fail because it forces buf to automatically start downloading the deps # bcode, bout, berr = run_command(f"{buf_path} mod update") # bout, berr = ''.join(bout), ''.join(berr) - target = Path(temp_dir.name, f"{Path(self._path_to_before).stem}.proto") buf_args = [ @@ -145,6 +197,12 @@ def run_detector(self) -> None: self.final_lock = final_lock def is_breaking(self) -> bool: + """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` + violate any breaking change rules specified by this breaking change detector. + + :return: a boolean flag indicating if the changes between ``before`` and ``after`` are breaking + :rtype: ``bool`` + """ final_code, final_out, final_err = self.final_result # Ways buf output could be indicative of a breaking change: @@ -155,4 +213,11 @@ def is_breaking(self) -> bool: return final_code != 0 or break_condition(final_out) or break_condition(final_err) def lock_file_changed(self) -> bool: + """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` + cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change + detector is checking all of the protobuf features we are interested in. + + :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file + :rtype: ``bool`` + """ return any(before != after for before, after in zip(self.initial_lock, self.final_lock)) From 741bbf314aa54bff986b7102a543f7b0810ef058 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Mon, 2 Aug 2021 23:23:52 +0000 Subject: [PATCH 12/16] update buf and fix format_pre check Signed-off-by: Yaseen Alkhafaji --- api/bazel/repository_locations.bzl | 6 +++--- generated_api_shadow/bazel/repository_locations.bzl | 6 +++--- tools/api_proto_breaking_change_detector/buf.yaml | 4 ++-- tools/api_proto_breaking_change_detector/detector.py | 8 +++++--- tools/api_proto_breaking_change_detector/detector_test.py | 2 +- .../allowed/test_add_comment_current | 2 +- .../allowed/test_add_comment_next | 2 +- .../allowed/test_add_enum_value_current | 2 +- .../allowed/test_add_enum_value_next | 2 +- .../allowed/test_add_field_current | 2 +- .../allowed/test_add_field_next | 2 +- .../allowed/test_add_option_current | 2 +- .../allowed/test_add_option_next | 2 +- .../allowed/test_force_breaking_change_current | 2 +- .../allowed/test_force_breaking_change_next | 2 +- .../allowed/test_remove_and_reserve_field_current | 2 +- .../allowed/test_remove_and_reserve_field_next | 4 ++-- .../breaking/test_change_field_from_oneof_current | 2 +- .../breaking/test_change_field_from_oneof_next | 2 +- .../breaking/test_change_field_id_current | 2 +- .../breaking/test_change_field_id_next | 2 +- .../breaking/test_change_field_name_current | 2 +- .../breaking/test_change_field_name_next | 2 +- .../breaking/test_change_field_plurality_current | 2 +- .../breaking/test_change_field_plurality_next | 2 +- .../breaking/test_change_field_to_oneof_current | 2 +- .../breaking/test_change_field_to_oneof_next | 2 +- .../breaking/test_change_field_type_current | 2 +- .../breaking/test_change_field_type_next | 2 +- .../breaking/test_change_package_name_current | 2 +- .../breaking/test_change_package_name_next | 2 +- .../breaking/test_change_pgv_oneof_current | 2 +- .../breaking/test_change_pgv_oneof_next | 2 +- 33 files changed, 43 insertions(+), 41 deletions(-) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index a2284503f8692..b004baf0c58c9 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -122,11 +122,11 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "buf", project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs project_url = "https://buf.build", - version = "0.46.0", - sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", + version = "0.48.2", + sha256 = "ee0ea6c4a7bbb016d79b056905c0a1f018e7c5e47b37038c993a77b1bc732c0d", strip_prefix = "buf", urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], - release_date = "2021-07-27", + release_date = "2021-07-30", use_category = ["api"], tags = ["manual"], ), diff --git a/generated_api_shadow/bazel/repository_locations.bzl b/generated_api_shadow/bazel/repository_locations.bzl index a2284503f8692..b004baf0c58c9 100644 --- a/generated_api_shadow/bazel/repository_locations.bzl +++ b/generated_api_shadow/bazel/repository_locations.bzl @@ -122,11 +122,11 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "buf", project_desc = "A new way of working with Protocol Buffers.", # Used for breaking change detection in API protobufs project_url = "https://buf.build", - version = "0.46.0", - sha256 = "4165fb44e5c0d6d5242e9f58bf53a31884a39e582abe1ce82dcb9d17c97ad54e", + version = "0.48.2", + sha256 = "ee0ea6c4a7bbb016d79b056905c0a1f018e7c5e47b37038c993a77b1bc732c0d", strip_prefix = "buf", urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"], - release_date = "2021-07-27", + release_date = "2021-07-30", use_category = ["api"], tags = ["manual"], ), diff --git a/tools/api_proto_breaking_change_detector/buf.yaml b/tools/api_proto_breaking_change_detector/buf.yaml index 7626ff1c7c8b1..f853642b37487 100644 --- a/tools/api_proto_breaking_change_detector/buf.yaml +++ b/tools/api_proto_breaking_change_detector/buf.yaml @@ -2,7 +2,7 @@ version: v1beta1 # commenting these out for now until I figure out how to automate buf.lock #deps: # TODO: should these point to specific commit hashes? - # + # # - buf.build/beta/udpa # - buf.build/beta/googleapis @@ -20,4 +20,4 @@ breaking: # needed for allowing removal/reserving of fields - FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED - - FIELD_NO_DELETE_UNLESS_NAME_RESERVED \ No newline at end of file + - FIELD_NO_DELETE_UNLESS_NAME_RESERVED diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 85ba797146ecb..2ec342782276c 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -66,7 +66,7 @@ def is_breaking(self) -> bool: @abstractmethod def lock_file_changed(self) -> bool: """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change + cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change detector is checking all of the protobuf features we are interested in. :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file @@ -209,12 +209,14 @@ def is_breaking(self) -> bool: # 1) final_code (exit status code) is not 0 (e.g. it's 100) # 2) stdout/stderr is nonempty # 3) stdout/stderr contains "Failure" - break_condition = lambda inp: len(inp) > 0 or bool(re.match(r"Failure", inp)) + def break_condition(inp): + return len(inp) > 0 or bool(re.match(r"Failure", inp)) + return final_code != 0 or break_condition(final_out) or break_condition(final_err) def lock_file_changed(self) -> bool: """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change + cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change detector is checking all of the protobuf features we are interested in. :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index 6bbe29d896d96..9c8fac543d883 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -1,7 +1,7 @@ from pathlib import Path import unittest -from detector import BufWrapper, ChangeDetectorInitializeError +from detector import BufWrapper """ Tests breaking change detectors (e.g. buf) against different proto changes to ensure proper behavior in `allowed` and `breaking` circumstances. Although the dependency likely already tests for these circumstances, diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current index 819ad5b8b9e22..ff7a78b1fc434 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_current @@ -6,4 +6,4 @@ option java_outer_classname = "EnvoyProtolock"; option java_multiple_files = true; message CommonExtensionConfig { -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next index 422405f5538b0..8bd44e75c89b8 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_comment_next @@ -7,4 +7,4 @@ option java_multiple_files = true; // Common configuration for all tap extensions. message CommonExtensionConfig { -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current index 89e9818cdd29f..0663f21f38ac6 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_current @@ -14,4 +14,4 @@ message SearchRequest { PRODUCTS = 5; } Corpus corpus = 4; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next index 53aac251a92dc..290aea11693ce 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_enum_value_next @@ -15,4 +15,4 @@ message SearchRequest { VIDEO = 6; } Corpus corpus = 4; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current index 0830a96e127c5..4a38563f01d97 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string three = 3; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next index af859a0cc5fbb..b17fe188e3fea 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_field_next @@ -4,4 +4,4 @@ package test.protos.breaking; message SampleMessage { string three = 3; float four = 4; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current index 2f0892d5d91b2..f6f3f59b856d7 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_current @@ -6,4 +6,4 @@ option java_outer_classname = "EnvoyProtolock"; // Common configuration for all tap extensions. message CommonExtensionConfig { -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next index 422405f5538b0..8bd44e75c89b8 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_add_option_next @@ -7,4 +7,4 @@ option java_multiple_files = true; // Common configuration for all tap extensions. message CommonExtensionConfig { -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current index 0830a96e127c5..4a38563f01d97 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string three = 3; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next index 89de3ac218470..2059fdb4f514e 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_force_breaking_change_next @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { float three = 3; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current index af859a0cc5fbb..b17fe188e3fea 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_current @@ -4,4 +4,4 @@ package test.protos.breaking; message SampleMessage { string three = 3; float four = 4; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next index 2607958e03119..f2ffa0510ad37 100644 --- a/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next +++ b/tools/testdata/api_proto_breaking_change_detector/allowed/test_remove_and_reserve_field_next @@ -4,6 +4,6 @@ package test.protos.breaking; message SampleMessage { reserved 4; reserved "four"; - + string three = 3; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current index 0a1e8f1064229..564601045ee9a 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_current @@ -7,4 +7,4 @@ message SampleMessage { string sub_message = 9; float special = 10; } -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next index 4e1ddfb1b47d4..b4965f64fd7ac 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_from_oneof_next @@ -7,4 +7,4 @@ message SampleMessage { string sub_message = 9; } float special = 10; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current index 0830a96e127c5..4a38563f01d97 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string three = 3; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next index 62babdd42530b..35984ed9637b9 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_id_next @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string three = 42; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current index 0645d218049c3..4471dbbb1a208 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string seventeen = 17; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next index 669225e3ee531..c6003d1384893 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_name_next @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string twenty = 17; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current index e1321b707cc2f..09e5e4374eeff 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string from = 1; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next index 2817267b2f292..63a5af85d0b24 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_plurality_next @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { repeated string from = 1; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current index 5d1debb193775..1bb452fdb9414 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_current @@ -7,4 +7,4 @@ message SampleMessage { string sub_message = 9; } float loner = 10; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next index b61743cef7e48..3336f4e384e92 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_to_oneof_next @@ -7,4 +7,4 @@ message SampleMessage { string sub_message = 9; float loner = 10; } -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current index f1cff9ac4483d..5cae750860e12 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { float pi = 314; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next index fe53195eef988..b235c8e224d0d 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_field_type_next @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string pi = 314; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current index 6f2260ac544e8..f48663822058d 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_current @@ -3,4 +3,4 @@ package test.protos.breaking; message SampleMessage { string passcode = 69; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next index da064e9bdfacf..867e36699374c 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_next @@ -3,4 +3,4 @@ package wrong.package; message SampleMessage { string passcode = 69; -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current index 47f17f9ec0228..266d6fee1cfd0 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_current @@ -7,4 +7,4 @@ message SampleMessage { string name = 4; string sub_message = 9;; } -} \ No newline at end of file +} diff --git a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next index 1fd45b1e2b58e..989642b4aca82 100644 --- a/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next +++ b/tools/testdata/api_proto_breaking_change_detector/breaking/test_change_pgv_oneof_next @@ -7,4 +7,4 @@ message SampleMessage { string name = 4; string sub_message = 9;; } -} \ No newline at end of file +} From 87491cb0db99f63cc73999f12932d1912c373b24 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Tue, 3 Aug 2021 00:19:53 +0000 Subject: [PATCH 13/16] context manager for temp dir Signed-off-by: Yaseen Alkhafaji --- .../detector.py | 124 +++++++++--------- 1 file changed, 61 insertions(+), 63 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 2ec342782276c..2451af24fd7c6 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -132,69 +132,67 @@ def run_detector(self) -> None: # 6) buf breaking --against tmp_file # 7) check for differences (if changes are breaking, there should be none) - temp_dir = tempfile.TemporaryDirectory( - prefix=str(Path(".").absolute()) - + os.sep) # buf requires protobuf files to be in a subdirectory of cwd - buf_path = runfiles.Create().Rlocation("com_github_bufbuild_buf/bin/buf") - - buf_config_loc = Path(".", "tools", "api_proto_breaking_change_detector") - - #copyfile(Path(buf_config_loc, "buf.lock"), Path(".", "buf.lock")) # not needed? refer to comment below - yaml_file_loc = Path(".", "buf.yaml") - copyfile(Path(buf_config_loc, "buf.yaml"), yaml_file_loc) - - # TODO: figure out how to automatically pull buf deps - # `buf mod update` doesn't seem to do anything, and the first test will fail because it forces buf to automatically start downloading the deps - # bcode, bout, berr = run_command(f"{buf_path} mod update") - # bout, berr = ''.join(bout), ''.join(berr) - target = Path(temp_dir.name, f"{Path(self._path_to_before).stem}.proto") - - buf_args = [ - "--path", - str(target.relative_to( - Path(".").absolute())), # buf requires relative pathing for roots... - "--config", - str(yaml_file_loc), - ] - if self._additional_args is not None: - buf_args.extend(self._additional_args) - - copyfile(self._path_to_before, target) - - initial_code, initial_out, initial_err = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) - initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) - - if initial_code != 0 or len(initial_out) > 0 or len(initial_err) > 0: - raise ChangeDetectorInitializeError( - f"Unexpected error during init:\n\tExit Status Code: {initial_code}\n\tstdout: {initial_out}\n\t stderr: {initial_err}\n" - ) - - lock_location = Path(temp_dir.name, BUF_STATE_FILE) - with open(lock_location, "r") as f: - initial_lock = f.readlines() - - copyfile(self._path_to_after, target) - - final_code, final_out, final_err = run_command( - ' '.join( - [buf_path, f"breaking --against {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) - final_out, final_err = ''.join(final_out), ''.join(final_err) - - # new with buf: lock must be manually re-built - # but here we only re-build if there weren't any detected breaking changes - if len(final_out) == len(final_err) == final_code == 0: - _, _, _ = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir.name, BUF_STATE_FILE)}", *buf_args])) - with open(lock_location, "r") as f: - final_lock = f.readlines() - - temp_dir.cleanup() - - self.initial_result = initial_code, initial_out, initial_err - self.final_result = final_code, final_out, final_err - self.initial_lock = initial_lock - self.final_lock = final_lock + with tempfile.TemporaryDirectory( + prefix=str(Path(".").absolute()) + + os.sep) as temp_dir: # buf requires protobuf files to be in a subdirectory of cwd + buf_path = runfiles.Create().Rlocation("com_github_bufbuild_buf/bin/buf") + + buf_config_loc = Path(".", "tools", "api_proto_breaking_change_detector") + + #copyfile(Path(buf_config_loc, "buf.lock"), Path(".", "buf.lock")) # not needed? refer to comment below + yaml_file_loc = Path(".", "buf.yaml") + copyfile(Path(buf_config_loc, "buf.yaml"), yaml_file_loc) + + # TODO: figure out how to automatically pull buf deps + # `buf mod update` doesn't seem to do anything, and the first test will fail because it forces buf to automatically start downloading the deps + # bcode, bout, berr = run_command(f"{buf_path} mod update") + # bout, berr = ''.join(bout), ''.join(berr) + target = Path(temp_dir, f"{Path(self._path_to_before).stem}.proto") + + buf_args = [ + "--path", + str(target.relative_to( + Path(".").absolute())), # buf requires relative pathing for roots... + "--config", + str(yaml_file_loc), + ] + if self._additional_args is not None: + buf_args.extend(self._additional_args) + + copyfile(self._path_to_before, target) + + initial_code, initial_out, initial_err = run_command( + ' '.join([buf_path, f"build -o {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) + + if initial_code != 0 or len(initial_out) > 0 or len(initial_err) > 0: + raise ChangeDetectorInitializeError( + f"Unexpected error during init:\n\tExit Status Code: {initial_code}\n\tstdout: {initial_out}\n\t stderr: {initial_err}\n" + ) + + lock_location = Path(temp_dir, BUF_STATE_FILE) + with open(lock_location, "r") as f: + initial_lock = f.readlines() + + copyfile(self._path_to_after, target) + + final_code, final_out, final_err = run_command( + ' '.join( + [buf_path, f"breaking --against {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + final_out, final_err = ''.join(final_out), ''.join(final_err) + + # new with buf: lock must be manually re-built + # but here we only re-build if there weren't any detected breaking changes + if len(final_out) == len(final_err) == final_code == 0: + _, _, _ = run_command( + ' '.join([buf_path, f"build -o {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + with open(lock_location, "r") as f: + final_lock = f.readlines() + + self.initial_result = initial_code, initial_out, initial_err + self.final_result = final_code, final_out, final_err + self.initial_lock = initial_lock + self.final_lock = final_lock def is_breaking(self) -> bool: """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` From e890a0bc48c3ebd5cd7e85df2b187b73bde3fe73 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 4 Aug 2021 23:51:31 +0000 Subject: [PATCH 14/16] address review comments and clean up python style Signed-off-by: Yaseen Alkhafaji --- .../api_proto_breaking_change_detector/BUILD | 38 +--- .../detector.py | 174 ++++++------------ .../detector_test.py | 28 ++- .../api_proto_breaking_change_detector/BUILD | 72 ++++---- 4 files changed, 109 insertions(+), 203 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/BUILD b/tools/api_proto_breaking_change_detector/BUILD index 9b6a50ffed08e..7eec29a9772b7 100644 --- a/tools/api_proto_breaking_change_detector/BUILD +++ b/tools/api_proto_breaking_change_detector/BUILD @@ -6,8 +6,8 @@ py_binary( name = "proto_breaking_change_detector", srcs = ["detector.py"], data = [ - "//tools/api_proto_breaking_change_detector:buf.lock", - "//tools/api_proto_breaking_change_detector:buf.yaml", + ":buf.lock", + ":buf.yaml", "@com_github_bufbuild_buf//:buf", ], main = "detector.py", @@ -21,45 +21,13 @@ py_test( name = "proto_breaking_change_detector_test", srcs = ["detector_test.py"], data = [ - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_comment_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_comment_next", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_enum_value_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_enum_value_next", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_field_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_field_next", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_option_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_add_option_next", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_force_breaking_change_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_force_breaking_change_next", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_remove_and_reserve_field_current", - "//tools/testdata/api_proto_breaking_change_detector:allowed/test_remove_and_reserve_field_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_from_oneof_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_from_oneof_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_id_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_id_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_name_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_name_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_plurality_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_plurality_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_to_oneof_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_to_oneof_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_type_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_field_type_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_package_name_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_package_name_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_field_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_field_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_message_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_message_next", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_oneof_current", - "//tools/testdata/api_proto_breaking_change_detector:breaking/test_change_pgv_oneof_next", + "//tools/testdata/api_proto_breaking_change_detector:proto_breaking_change_detector_testdata", "@com_github_bufbuild_buf//:buf", ], main = "detector_test.py", python_version = "PY3", srcs_version = "PY3", tags = ["manual"], - visibility = ["//visibility:public"], deps = [ ":proto_breaking_change_detector", "@rules_python//python/runfiles", diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 2451af24fd7c6..9711b1490dc56 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -1,76 +1,67 @@ -from abc import ABC, abstractmethod +""" Protocol Buffer Breaking Change Detector + +This tool is used to detect "breaking changes" in protobuf files, to +ensure proper backwards-compatibility in protobuf API updates. The tool +can check for breaking changes of a single API by taking 2 .proto file +paths as input (before and after) and outputting a bool `is_breaking`. + +The breaking change detector creates a temporary directory, copies in +each file to compute a protobuf "state", computes a diff of the "before" +and "after" states, and runs the diff against a set of rules to determine +if there was a breaking change. + +The tool is currently implemented with buf (https://buf.build/) +""" + import tempfile from rules_python.python.runfiles import runfiles from tools.run_command import run_command from shutil import copyfile -import re from pathlib import Path import os from typing import List -""" -This tool is used to detect "breaking changes" in protobuf files, to ensure proper backwards-compatibility in protobuf API updates. -The tool can check for breaking changes of a single API by taking 2 .proto file paths as input (before and after) and outputting a bool `is_breaking`. -The breaking change detector creates a temporary directory, copies in each file to compute a protobuf "state", -computes a diff of the "before" and "after" states, and runs the diff against a set of rules to determine if there was a breaking change. -The tool is currently implemented with buf (https://buf.build/) -""" - - -# generic breaking change detector for protos, extended by a wrapper class for a breaking change detector -class ProtoBreakingChangeDetector(ABC): - - @abstractmethod - def __init__( - self, - path_to_before: str, - path_to_after: str, - additional_args: List[str] = None) -> None: - """Initializes the breaking change detector, setting up any necessary config without actually running - the detector against any proto files. +class ProtoBreakingChangeDetector(object): + """Abstract breaking change detector interface""" - Takes in a single protobuf as 2 files, in a ``before`` state and an ``after`` state, - and checks if the ``after`` state violates any breaking change rules. + def __init__(self, path_to_before: str, path_to_after: str) -> None: + """Initialize the configuration of the breaking change detector - :param path_to_before: absolute path to the .proto file in the before state - :type path_to_before: ``str`` + This function sets up any necessary config without actually + running the detector against any proto files. - :param path_to_after: absolute path to the .proto file in the after state - :type path_to_after: ``str`` + Takes in a single protobuf as 2 files, in a ``before`` state + and an ``after`` state, and checks if the ``after`` state + violates any breaking change rules. - :param additional_args: additional arguments for the breaking change detector CLI. May be tool-dependent. - Additional arguments are passed in both the "initialize" and "detect changes" steps. - :type additional_args: ``List[str]`` + Args: + path_to_before {str} -- absolute path to the .proto file in the before state + path_to_after {str} -- absolute path to the .proto file in the after state """ pass - @abstractmethod def run_detector(self) -> None: - """Runs the breaking change detector with the parameters given in ``__init__``. This method should populate - the detector's internal data such that `is_breaking` and `lock_file_changed` do not require any additional - invocations to the breaking change detector. + """Run the breaking change detector to detect rule violations + + This method should populate the detector's internal data such + that `is_breaking` and `lock_file_changed` do not require any + additional invocations to the breaking change detector. """ pass - @abstractmethod def is_breaking(self) -> bool: - """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - violate any breaking change rules specified by this breaking change detector. - - :return: a boolean flag indicating if the changes between ``before`` and ``after`` are breaking - :rtype: ``bool`` - """ + """Return True if breaking changes were detected in the given protos""" pass - @abstractmethod def lock_file_changed(self) -> bool: - """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change - detector is checking all of the protobuf features we are interested in. + """Return True if the detector state file changed after being run - :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file - :rtype: ``bool`` + This function assumes that the detector uses a lock file to + compare "before" and "after" states of protobufs, which is + admittedly an implementation detail. It is mostly used for + testing, to ensure that the breaking change detector is + checking all of the protobuf features we are interested in. """ pass @@ -87,28 +78,13 @@ class ChangeDetectorInitializeError(ChangeDetectorError): class BufWrapper(ProtoBreakingChangeDetector): + """Breaking change detector implemented with buf""" def __init__( self, path_to_before: str, path_to_after: str, additional_args: List[str] = None) -> None: - """Initializes the breaking change detector, setting up any necessary config without actually running - the detector against any proto files. - - Takes in a single protobuf as 2 files, in a ``before`` state and an ``after`` state, - and checks if the ``after`` state violates any breaking change rules. - - :param path_to_before: absolute path to the .proto file in the before state - :type path_to_before: ``str`` - - :param path_to_after: absolute path to the .proto file in the after state - :type path_to_after: ``str`` - - :param additional_args: additional arguments for the breaking change detector CLI. May be tool-dependent. - Additional arguments are passed in both the "initialize" and "detect changes" steps. - :type additional_args: ``List[str]`` - """ if not Path(path_to_before).is_file(): raise ValueError(f"path_to_before {path_to_before} does not exist") @@ -120,49 +96,32 @@ def __init__( self._additional_args = additional_args def run_detector(self) -> None: - """Runs the breaking change detector with the parameters given in ``__init__``. This method should populate - the detector's internal data such that `is_breaking` and `lock_file_changed` do not require any additional - invocations to the breaking change detector. - """ - # 1) pull buf BSR dependencies with buf mod update (? still need to figure this out. commented out for now) - # 2) copy buf.yaml into temp dir - # 3) copy start file into temp dir - # 4) buf build -o tmp_file - # 5) copy changed file into temp dir - # 6) buf breaking --against tmp_file - # 7) check for differences (if changes are breaking, there should be none) - - with tempfile.TemporaryDirectory( - prefix=str(Path(".").absolute()) - + os.sep) as temp_dir: # buf requires protobuf files to be in a subdirectory of cwd + # buf requires protobuf files to be in a subdirectory of the yaml file + with tempfile.TemporaryDirectory(prefix=str(Path(".").absolute()) + os.sep) as temp_dir: buf_path = runfiles.Create().Rlocation("com_github_bufbuild_buf/bin/buf") buf_config_loc = Path(".", "tools", "api_proto_breaking_change_detector") - #copyfile(Path(buf_config_loc, "buf.lock"), Path(".", "buf.lock")) # not needed? refer to comment below yaml_file_loc = Path(".", "buf.yaml") copyfile(Path(buf_config_loc, "buf.yaml"), yaml_file_loc) - # TODO: figure out how to automatically pull buf deps - # `buf mod update` doesn't seem to do anything, and the first test will fail because it forces buf to automatically start downloading the deps - # bcode, bout, berr = run_command(f"{buf_path} mod update") - # bout, berr = ''.join(bout), ''.join(berr) target = Path(temp_dir, f"{Path(self._path_to_before).stem}.proto") buf_args = [ "--path", - str(target.relative_to( - Path(".").absolute())), # buf requires relative pathing for roots... + # buf requires relative pathing for roots + str(target.relative_to(Path(".").absolute())), "--config", str(yaml_file_loc), ] - if self._additional_args is not None: - buf_args.extend(self._additional_args) + buf_args.extend(self._additional_args or []) copyfile(self._path_to_before, target) + lock_location = Path(temp_dir, BUF_STATE_FILE) + initial_code, initial_out, initial_err = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + ' '.join([buf_path, f"build -o {lock_location}", *buf_args])) initial_out, initial_err = ''.join(initial_out), ''.join(initial_err) if initial_code != 0 or len(initial_out) > 0 or len(initial_err) > 0: @@ -170,22 +129,17 @@ def run_detector(self) -> None: f"Unexpected error during init:\n\tExit Status Code: {initial_code}\n\tstdout: {initial_out}\n\t stderr: {initial_err}\n" ) - lock_location = Path(temp_dir, BUF_STATE_FILE) with open(lock_location, "r") as f: initial_lock = f.readlines() copyfile(self._path_to_after, target) final_code, final_out, final_err = run_command( - ' '.join( - [buf_path, f"breaking --against {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + ' '.join([buf_path, f"breaking --against {lock_location}", *buf_args])) final_out, final_err = ''.join(final_out), ''.join(final_err) - # new with buf: lock must be manually re-built - # but here we only re-build if there weren't any detected breaking changes if len(final_out) == len(final_err) == final_code == 0: - _, _, _ = run_command( - ' '.join([buf_path, f"build -o {Path(temp_dir, BUF_STATE_FILE)}", *buf_args])) + _, _, _ = run_command(' '.join([buf_path, f"build -o {lock_location}", *buf_args])) with open(lock_location, "r") as f: final_lock = f.readlines() @@ -195,29 +149,15 @@ def run_detector(self) -> None: self.final_lock = final_lock def is_breaking(self) -> bool: - """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - violate any breaking change rules specified by this breaking change detector. - - :return: a boolean flag indicating if the changes between ``before`` and ``after`` are breaking - :rtype: ``bool`` - """ final_code, final_out, final_err = self.final_result - # Ways buf output could be indicative of a breaking change: - # 1) final_code (exit status code) is not 0 (e.g. it's 100) - # 2) stdout/stderr is nonempty - # 3) stdout/stderr contains "Failure" - def break_condition(inp): - return len(inp) > 0 or bool(re.match(r"Failure", inp)) - - return final_code != 0 or break_condition(final_out) or break_condition(final_err) + if final_code != 0: + return True + if final_out != "" or "Failure" in final_out: + return True + if final_err != "" or "Failure" in final_err: + return True + return False def lock_file_changed(self) -> bool: - """Returns whether the changes between the ``before`` and ``after`` states of the proto file given in ``__init__`` - cause a change in the detector's state file. This function is mostly used for testing, to ensure that the breaking change - detector is checking all of the protobuf features we are interested in. - - :return: a boolean flag indicating if the changes between ``before`` and ``after`` cause a change in the state file - :rtype: ``bool`` - """ return any(before != after for before, after in zip(self.initial_lock, self.final_lock)) diff --git a/tools/api_proto_breaking_change_detector/detector_test.py b/tools/api_proto_breaking_change_detector/detector_test.py index 9c8fac543d883..874ad4b788bc3 100644 --- a/tools/api_proto_breaking_change_detector/detector_test.py +++ b/tools/api_proto_breaking_change_detector/detector_test.py @@ -1,18 +1,23 @@ +""" Proto Breaking Change Detector Test Suite + +This script evaluates breaking change detectors (e.g. buf) against +different protobuf file changes to ensure proper and consistent behavior +in `allowed`and `breaking` circumstances. Although the dependency likely +already tests for these circumstances, these specify Envoy's requirements +and ensure that tool behavior is consistent across dependency updates. +""" + from pathlib import Path import unittest from detector import BufWrapper -""" -Tests breaking change detectors (e.g. buf) against different proto changes to ensure proper behavior in -`allowed` and `breaking` circumstances. Although the dependency likely already tests for these circumstances, -these specify Envoy's requirements and ensure that tool behavior is consistent across dependency updates. -""" class BreakingChangeDetectorTests(object): detector_type = None def run_detector_test(self, testname, is_breaking, expects_changes, additional_args=None): + """Runs a test case for an arbitrary breaking change detector type""" tests_path = Path( Path(__file__).absolute().parent.parent, "testdata", "api_proto_breaking_change_detector", "breaking" if is_breaking else "allowed") @@ -68,7 +73,7 @@ def test_change_pgv_oneof(self): class TestAllowedChanges(BreakingChangeDetectorTests): - def run_allowed_test(self, testname, additional_args=None): + def run_allowed_test(self, testname): self.run_detector_test(testname, is_breaking=False, expects_changes=True) def test_add_comment(self): @@ -86,9 +91,6 @@ def test_add_enum_value(self): def test_remove_and_reserve_field(self): self.run_allowed_test(self.test_remove_and_reserve_field.__name__) - def test_force_breaking_change(self): - self.run_allowed_test(self.test_force_breaking_change.__name__, additional_args=["--force"]) - class BufTests(TestAllowedChanges, TestBreakingChanges, unittest.TestCase): detector_type = BufWrapper @@ -105,14 +107,6 @@ def test_change_pgv_message(self): def test_change_pgv_oneof(self): pass - # copied from protolock tests but might remove - # It doesn't make sense to evaluate 'forcing' a breaking change in buf because by default, - # buf lets you re-build without checking for breaking changes - # Buf does not require forcing breaking changes into the lock file like protolock does - @unittest.skip("'forcing' a breaking change does not make sense for buf") - def test_force_breaking_change(self): - pass - if __name__ == '__main__': unittest.main() diff --git a/tools/testdata/api_proto_breaking_change_detector/BUILD b/tools/testdata/api_proto_breaking_change_detector/BUILD index e9e797dca8c6f..5fb56cefbd2b4 100644 --- a/tools/testdata/api_proto_breaking_change_detector/BUILD +++ b/tools/testdata/api_proto_breaking_change_detector/BUILD @@ -1,36 +1,40 @@ licenses(["notice"]) # Apache 2 -exports_files([ - "allowed/test_add_comment_current", - "allowed/test_add_enum_value_next", - "allowed/test_add_option_current", - "allowed/test_force_breaking_change_next", - "allowed/test_add_comment_next", - "allowed/test_add_field_current", - "allowed/test_add_option_next", - "allowed/test_remove_and_reserve_field_current", - "allowed/test_add_enum_value_current", - "allowed/test_add_field_next", - "allowed/test_force_breaking_change_current", - "allowed/test_remove_and_reserve_field_next", - "breaking/test_change_field_from_oneof_current", - "breaking/test_change_field_name_next", - "breaking/test_change_field_type_current", - "breaking/test_change_pgv_field_next", - "breaking/test_change_field_from_oneof_next", - "breaking/test_change_field_plurality_current", - "breaking/test_change_field_type_next", - "breaking/test_change_pgv_message_current", - "breaking/test_change_field_id_current", - "breaking/test_change_field_plurality_next", - "breaking/test_change_package_name_current", - "breaking/test_change_pgv_message_next", - "breaking/test_change_field_id_next", - "breaking/test_change_field_to_oneof_current", - "breaking/test_change_package_name_next", - "breaking/test_change_pgv_oneof_current", - "breaking/test_change_field_name_current", - "breaking/test_change_field_to_oneof_next", - "breaking/test_change_pgv_field_current", - "breaking/test_change_pgv_oneof_next", -]) +filegroup( + name = "proto_breaking_change_detector_testdata", + srcs = [ + "allowed/test_add_comment_current", + "allowed/test_add_comment_next", + "allowed/test_add_enum_value_current", + "allowed/test_add_enum_value_next", + "allowed/test_add_field_current", + "allowed/test_add_field_next", + "allowed/test_add_option_current", + "allowed/test_add_option_next", + "allowed/test_force_breaking_change_current", + "allowed/test_force_breaking_change_next", + "allowed/test_remove_and_reserve_field_current", + "allowed/test_remove_and_reserve_field_next", + "breaking/test_change_field_from_oneof_current", + "breaking/test_change_field_from_oneof_next", + "breaking/test_change_field_id_current", + "breaking/test_change_field_id_next", + "breaking/test_change_field_name_current", + "breaking/test_change_field_name_next", + "breaking/test_change_field_plurality_current", + "breaking/test_change_field_plurality_next", + "breaking/test_change_field_to_oneof_current", + "breaking/test_change_field_to_oneof_next", + "breaking/test_change_field_type_current", + "breaking/test_change_field_type_next", + "breaking/test_change_package_name_current", + "breaking/test_change_package_name_next", + "breaking/test_change_pgv_field_current", + "breaking/test_change_pgv_field_next", + "breaking/test_change_pgv_message_current", + "breaking/test_change_pgv_message_next", + "breaking/test_change_pgv_oneof_current", + "breaking/test_change_pgv_oneof_next", + ], + visibility = ["//visibility:public"], +) From c86185005c7fb21dc8f4103b559b5ce5be435482 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Tue, 10 Aug 2021 16:41:03 +0000 Subject: [PATCH 15/16] address more pr comments Signed-off-by: Yaseen Alkhafaji --- api/bazel/repositories.bzl | 2 +- generated_api_shadow/bazel/repositories.bzl | 2 +- .../api_proto_breaking_change_detector/detector.py | 14 ++++++-------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index 5f8cca8fcd0a3..ef92aa45f0064 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -166,6 +166,6 @@ filegroup( srcs = [ "@com_github_bufbuild_buf//:bin/buf", ], - tags = ["manual"], + tags = ["manual"], # buf is downloaded as a linux binary; tagged manual to prevent build for non-linux users ) """ diff --git a/generated_api_shadow/bazel/repositories.bzl b/generated_api_shadow/bazel/repositories.bzl index 5f8cca8fcd0a3..ef92aa45f0064 100644 --- a/generated_api_shadow/bazel/repositories.bzl +++ b/generated_api_shadow/bazel/repositories.bzl @@ -166,6 +166,6 @@ filegroup( srcs = [ "@com_github_bufbuild_buf//:bin/buf", ], - tags = ["manual"], + tags = ["manual"], # buf is downloaded as a linux binary; tagged manual to prevent build for non-linux users ) """ diff --git a/tools/api_proto_breaking_change_detector/detector.py b/tools/api_proto_breaking_change_detector/detector.py index 9711b1490dc56..88fe99ed193a8 100644 --- a/tools/api_proto_breaking_change_detector/detector.py +++ b/tools/api_proto_breaking_change_detector/detector.py @@ -130,7 +130,7 @@ def run_detector(self) -> None: ) with open(lock_location, "r") as f: - initial_lock = f.readlines() + self._initial_lock = f.readlines() copyfile(self._path_to_after, target) @@ -141,15 +141,13 @@ def run_detector(self) -> None: if len(final_out) == len(final_err) == final_code == 0: _, _, _ = run_command(' '.join([buf_path, f"build -o {lock_location}", *buf_args])) with open(lock_location, "r") as f: - final_lock = f.readlines() + self._final_lock = f.readlines() - self.initial_result = initial_code, initial_out, initial_err - self.final_result = final_code, final_out, final_err - self.initial_lock = initial_lock - self.final_lock = final_lock + self._initial_result = initial_code, initial_out, initial_err + self._final_result = final_code, final_out, final_err def is_breaking(self) -> bool: - final_code, final_out, final_err = self.final_result + final_code, final_out, final_err = self._final_result if final_code != 0: return True @@ -160,4 +158,4 @@ def is_breaking(self) -> bool: return False def lock_file_changed(self) -> bool: - return any(before != after for before, after in zip(self.initial_lock, self.final_lock)) + return any(before != after for before, after in zip(self._initial_lock, self._final_lock)) From edceb74e072b007ebe5527f0d16d31510185ba34 Mon Sep 17 00:00:00 2001 From: Yaseen Alkhafaji Date: Wed, 11 Aug 2021 00:46:50 +0000 Subject: [PATCH 16/16] remove misc comments Signed-off-by: Yaseen Alkhafaji --- tools/api_proto_breaking_change_detector/buf.yaml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tools/api_proto_breaking_change_detector/buf.yaml b/tools/api_proto_breaking_change_detector/buf.yaml index f853642b37487..2f3631072e7c4 100644 --- a/tools/api_proto_breaking_change_detector/buf.yaml +++ b/tools/api_proto_breaking_change_detector/buf.yaml @@ -1,14 +1,4 @@ version: v1beta1 -# commenting these out for now until I figure out how to automate buf.lock -#deps: - # TODO: should these point to specific commit hashes? - # - -# - buf.build/beta/udpa -# - buf.build/beta/googleapis -# - buf.build/beta/opencensus -# - buf.build/beta/prometheus -# - buf.build/beta/opentelemetry breaking: use: - FIELD_SAME_ONEOF