-
Notifications
You must be signed in to change notification settings - Fork 5.4k
tooling: Add buf bazel dependency and tests to evaluate it #17515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
ee1782f
Add buf bazel dependency and tests to evaluate it
YaseenAlk 8574b7e
check exit code too
YaseenAlk 29e7dd2
small test cleanup
YaseenAlk 2f9fdef
make tests detector-independent
YaseenAlk cf0173a
add comment describing tool behavior
YaseenAlk 22efce4
make detector class stateful
YaseenAlk f402d61
comment on test suite
YaseenAlk 21f1456
move buf bazel repo to api/bazel
YaseenAlk ff59f6f
fix nits from second pass
YaseenAlk ec1b3de
add type hints to ProtoBreakingChangeDetector api
YaseenAlk 5640665
document ProtoBreakingChangeDetector api funcs
YaseenAlk 741bbf3
update buf and fix format_pre check
YaseenAlk 87491cb
context manager for temp dir
YaseenAlk 71371fd
Merge branch 'main' into buf_migration_pr
YaseenAlk e890a0b
address review comments and clean up python style
YaseenAlk c861850
address more pr comments
YaseenAlk edceb74
remove misc comments
YaseenAlk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| 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 = [ | ||
| ":buf.lock", | ||
| ":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:proto_breaking_change_detector_testdata", | ||
| "@com_github_bufbuild_buf//:buf", | ||
| ], | ||
| main = "detector_test.py", | ||
| python_version = "PY3", | ||
| srcs_version = "PY3", | ||
| tags = ["manual"], | ||
| deps = [ | ||
| ":proto_breaking_change_detector", | ||
| "@rules_python//python/runfiles", | ||
| ], | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| version: v1beta1 | ||
| 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| """ 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 | ||
| from pathlib import Path | ||
| import os | ||
| from typing import List | ||
|
|
||
|
|
||
| class ProtoBreakingChangeDetector(object): | ||
| """Abstract breaking change detector interface""" | ||
|
|
||
| def __init__(self, path_to_before: str, path_to_after: str) -> None: | ||
| """Initialize the configuration of the breaking change detector | ||
|
|
||
| This function sets 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. | ||
|
|
||
| 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 | ||
|
|
||
| def run_detector(self) -> None: | ||
| """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 | ||
|
|
||
| def is_breaking(self) -> bool: | ||
| """Return True if breaking changes were detected in the given protos""" | ||
| pass | ||
|
|
||
| def lock_file_changed(self) -> bool: | ||
| """Return True if the detector state file changed after being run | ||
|
|
||
| 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 | ||
|
|
||
|
|
||
| class ChangeDetectorError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class ChangeDetectorInitializeError(ChangeDetectorError): | ||
| pass | ||
|
|
||
|
|
||
| BUF_STATE_FILE = "tmp.json" | ||
|
|
||
|
|
||
| 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: | ||
| 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) -> None: | ||
| # 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") | ||
|
|
||
| yaml_file_loc = Path(".", "buf.yaml") | ||
| copyfile(Path(buf_config_loc, "buf.yaml"), yaml_file_loc) | ||
|
|
||
| target = Path(temp_dir, f"{Path(self._path_to_before).stem}.proto") | ||
|
|
||
| buf_args = [ | ||
| "--path", | ||
| # buf requires relative pathing for roots | ||
| str(target.relative_to(Path(".").absolute())), | ||
| "--config", | ||
| str(yaml_file_loc), | ||
| ] | ||
| 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 {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: | ||
| raise ChangeDetectorInitializeError( | ||
| f"Unexpected error during init:\n\tExit Status Code: {initial_code}\n\tstdout: {initial_out}\n\t stderr: {initial_err}\n" | ||
| ) | ||
|
|
||
| with open(lock_location, "r") as f: | ||
| self._initial_lock = f.readlines() | ||
|
|
||
| copyfile(self._path_to_after, target) | ||
|
|
||
| final_code, final_out, final_err = run_command( | ||
| ' '.join([buf_path, f"breaking --against {lock_location}", *buf_args])) | ||
| final_out, final_err = ''.join(final_out), ''.join(final_err) | ||
|
|
||
| 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: | ||
| self._final_lock = f.readlines() | ||
|
|
||
| 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 | ||
|
|
||
| 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: | ||
| return any(before != after for before, after in zip(self._initial_lock, self._final_lock)) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the list of breaking change rules we're currently using
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we'd recommend using one of the categories to match what you want https://docs.buf.build/breaking-rules instead of choosing individual rules. Note that for a given configuration version, the rules within each category are stable, i.e. if you choose
PACKAGEorWIRE_JSON(which look to be roughly what you're going for), we won't add new rules and break your code as long asversionisv1beta1.