ci: Add API protobuf breaking-change-detector CI script #17793

YaseenAlk · 2021-08-20T19:45:29Z

Commit Message: Add API protobuf breaking-change-detector CI script

Additional Description:

This PR is a continuation to #17515 - it adds a script that uses buf to check for breaking changes on proto files in the api folder. It does so by comparing the current api folder against the api folder at the git commit computed by tools/git/last_github_commit.sh - that should hopefully represent the most recent commit on main (if there is a better method to obtain the base branch commit, let me know!).

Adding the script also required re-organizing some of the breaking change detector logic from the previous pr: some levels of abstraction were added, and the detector now expects a git repository and ref as the input for initial state (rather than a proto file).

The script is invoked in do_ci.sh if bazel.api_compat is specified as the CI_TARGET.

This PR also bumps the buf bazel dependency to 0.53.0. If this is preferred to be in a separate PR, let me know and I would be happy to do so

Risk Level: low (hopefully) - the CI script will be invoked in a separate CI pipeline job that can be set to be optional on github. The azure pipeline has been added but needs to be set to optional by a CI maintainer

Testing: New scripts and logic were tested manually; also ran tests from the previous PR and they still pass. hoping to observe more output of this tool through reading CI logs of other PRs once this is merged (this PR should not affect the existing PR workflow - refer to Risk Level)

Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: CI script uses a linux binary for buf so it cannot be run outside of docker on non-linux systems

Related Issue #3368

Signed-off-by: Yaseen Alkhafaji <[email protected]>

repokitteh-read-only · 2021-08-20T19:45:34Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17793 was opened by YaseenAlk.

see: more, trace.

YaseenAlk · 2021-08-20T19:47:44Z

cc @akonradi @adisuissa

Signed-off-by: Yaseen Alkhafaji <[email protected]>

akonradi

Still need to review plenty of the code but flushing comments for now

tools/api_proto_breaking_change_detector/BUILD

tools/api_proto_breaking_change_detector/buf_utils.py

akonradi · 2021-08-20T20:47:42Z

tools/api_proto_breaking_change_detector/buf_utils.py

+    buf_args = _generate_buf_args(target_path, config_file_loc, additional_args)
+
+    update_code, _, update_err = run_command(f'{buf_path} mod update')
+    # for some reason buf prints out the "downloading..." lines on stderr


We can capture or redirect stderr if that's useful?

tools/api_proto_breaking_change_detector/buf_utils.py

tools/api_proto_breaking_change_detector/detector.py

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk · 2021-08-21T21:40:41Z

/retest

repokitteh-read-only · 2021-08-21T21:40:45Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17793 (comment) was created by @YaseenAlk.

see: more, trace.

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk · 2021-08-24T03:18:43Z

/retest

repokitteh-read-only · 2021-08-24T03:18:46Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17793 (comment) was created by @YaseenAlk.

see: more, trace.

adisuissa

Thanks for working on this!
Left a few high-level comments

tools/api_proto_breaking_change_detector/detector.py

adisuissa · 2021-08-24T13:47:42Z

tools/api_proto_breaking_change_detector/BUILD

@@ -1,28 +1,48 @@
-load("@rules_python//python:defs.bzl", "py_binary", "py_test")
+load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test")

 licenses(["notice"])  # Apache 2

 py_binary(


Maybe we should also have detector_ci.py here.

tools/api_proto_breaking_change_detector/detector.py

akonradi

Happy to re-review once the changes to remove lock file support are made

tools/api_proto_breaking_change_detector/BUILD

tools/api_proto_breaking_change_detector/buf_utils.py

tools/api_proto_breaking_change_detector/detector_ci.py

tools/api_proto_breaking_change_detector/detector_test.py

tools/base/utils.py

htuch · 2021-08-26T19:41:14Z

ci/do_ci.sh

@@ -374,6 +374,16 @@ elif [[ "$CI_TARGET" == "bazel.api" ]]; then
  echo "Testing API boosting (golden C++ tests)..."
  # We use custom BAZEL_BUILD_OPTIONS here; the API booster isn't capable of working with libc++ yet.
  BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS[*]}" python3.8 "${ENVOY_SRCDIR}"/tools/api_boost/api_boost_test.py
+  echo "Building buf..."


I'd recommend moving this to a bazel.api_compat target and CI job. We can then turn this on as an optional GitHub check, which will make it an active part of the ongoing review process. If we see false positives or need to override, it won't block developer velocity, and ideally we can tune them. At some point it can become a mandatory check and senior maintainers can override as needed.

WDYT @lizan @phlax @mattklein123?

I'm very excited to see this land and think this will be immeasurably valuable in avoiding costly breaking changes that hurt the ecosystem. It will also free up bandwidth of @envoyproxy/api-shepherds to focus on things other than breaking changes.

not sure what adding a new CI job entails on the azure/github side but I modified .azure-pipelines/pipelines.yml to add api_compat under api for the bazel job of the check stage. if I need to do anything else, let me know

I see the new CI check, and it seems to be failing as expected.

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk · 2021-08-27T07:08:21Z

this should be ready for a second review! @adisuissa @akonradi

akonradi

LGTM with one nit.

akonradi · 2021-08-27T14:19:02Z

ci/do_ci.sh

@@ -374,6 +374,16 @@ elif [[ "$CI_TARGET" == "bazel.api" ]]; then
  echo "Testing API boosting (golden C++ tests)..."
  # We use custom BAZEL_BUILD_OPTIONS here; the API booster isn't capable of working with libc++ yet.
  BAZEL_BUILD_OPTIONS="${BAZEL_BUILD_OPTIONS[*]}" python3.8 "${ENVOY_SRCDIR}"/tools/api_boost/api_boost_test.py
+  echo "Building buf..."


I see the new CI check, and it seems to be failing as expected.

tools/api_proto_breaking_change_detector/detector.py

tools/testdata/api_proto_breaking_change_detector/breaking/test_change_package_name_changed

Signed-off-by: Yaseen Alkhafaji <[email protected]>

adisuissa

LGTM, thanks!

adisuissa · 2021-08-27T17:16:57Z

Please add a comment on why api_compat is failing for this PR.

johanbrandhorst · 2021-08-27T17:19:40Z

api/buf.yaml

@@ -1,13 +1,19 @@
 version: v1beta1
+deps:
+    - buf.build/beta/googleapis


FYI, this protobuf module has been deprecated in favor of buf.build/googleapis/googleapis. I think it'd be good to update it now before merging this as changing the dependency later can be annoying for downstream dependents.

YaseenAlk · 2021-08-27T17:20:43Z

@htuch this should be ready for one last review from you! the api_compat check will fail until we merge in necessary buf config files into the main branch (e.g. buf.lock file), because buf is trying to compile the API proto files on the main branch without proper configuration. I will also monitor other PRs today to make sure it starts passing and I can try to make a quick fix if something else is wrong

johanbrandhorst · 2021-08-27T17:37:28Z

tools/api_proto_breaking_change_detector/buf_utils.py

+            initial_state_input += f',subdir={subdir}'
+
+        final_code, final_out, final_err = run_command(
+            ' '.join([buf_path, f"breaking --against {initial_state_input}", *buf_args]))


FYI, if you actually want to parse this output (I see it's not really being parsed here) you can use --error-format=json to get structured results.

YaseenAlk · 2021-08-27T17:37:47Z

actually hold on, going to push one more change to update buf version and adjust BSR dependencies that @johanbrandhorst recommended

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk · 2021-08-27T18:18:34Z

/retest

repokitteh-read-only · 2021-08-27T18:18:37Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17793 (comment) was created by @YaseenAlk.

see: more, trace.

htuch

LGTM, thanks! Look forward to working with this and iterating. Great accomplishment for the internship!

htuch · 2021-08-27T21:03:44Z

/lgtm deps

htuch · 2021-08-27T21:05:33Z

@envoyproxy/maintainers @phlax this may cause issues in CI or PRs; presumably not given its an optional check, but just flagging it in case. Please rollback if there is a genuine CI breakage (i.e. modulo the optional api_compat check).

YaseenAlk added 3 commits August 20, 2021 18:35

Add breaking change detector CI script

b088659

Signed-off-by: Yaseen Alkhafaji <[email protected]>

update buf to 0.52.0

ba516ac

Signed-off-by: Yaseen Alkhafaji <[email protected]>

remove leftover buf.yaml file

6003e14

Signed-off-by: Yaseen Alkhafaji <[email protected]>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Aug 20, 2021

YaseenAlk mentioned this pull request Aug 20, 2021

tooling: Add buf bazel dependency and tests to evaluate it #17515

Merged

fix format_pre errors

05a648a

Signed-off-by: Yaseen Alkhafaji <[email protected]>

akonradi suggested changes Aug 20, 2021

View reviewed changes

YaseenAlk added 2 commits August 21, 2021 03:13

address first wave of pr comments

256c014

Signed-off-by: Yaseen Alkhafaji <[email protected]>

fix format_pre again

2713fb5

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk added 2 commits August 22, 2021 21:26

cd during all buf ops to fix ci

1dff015

Signed-off-by: Yaseen Alkhafaji <[email protected]>

pulling buf deps gives a nonzero error code

f41bbf4

Signed-off-by: Yaseen Alkhafaji <[email protected]>

phlax assigned akonradi, htuch and phlax and unassigned htuch Aug 23, 2021

YaseenAlk mentioned this pull request Aug 23, 2021

rocketmq: fix protobuf import path #17810

Closed

YaseenAlk added 2 commits August 23, 2021 18:13

also echo base commit in ci script

df31d13

Signed-off-by: Yaseen Alkhafaji <[email protected]>

fix format_pre again

2688dbd

Signed-off-by: Yaseen Alkhafaji <[email protected]>

adisuissa self-assigned this Aug 24, 2021

adisuissa reviewed Aug 24, 2021

View reviewed changes

akonradi suggested changes Aug 24, 2021

View reviewed changes

htuch reviewed Aug 26, 2021

View reviewed changes

YaseenAlk added 2 commits August 27, 2021 06:17

remove lock mode from bufwrapper and tests

03b871f

Signed-off-by: Yaseen Alkhafaji <[email protected]>

address pr comments and hopefully fix format_pre

a6eb008

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk added 2 commits August 27, 2021 06:50

try adding new azure job for api compat check

c6a7a94

Signed-off-by: Yaseen Alkhafaji <[email protected]>

fix format again

a213931

Signed-off-by: Yaseen Alkhafaji <[email protected]>

akonradi previously approved these changes Aug 27, 2021

View reviewed changes

fix bug so ci script only looks at api folder

f5981ae

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk dismissed akonradi’s stale review via f5981ae August 27, 2021 15:10

adisuissa previously approved these changes Aug 27, 2021

View reviewed changes

johanbrandhorst reviewed Aug 27, 2021

View reviewed changes

update buf to v0.53.0 and update BSR deps

2659202

Signed-off-by: Yaseen Alkhafaji <[email protected]>

YaseenAlk dismissed adisuissa’s stale review via 2659202 August 27, 2021 17:41

htuch approved these changes Aug 27, 2021

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 27, 2021

htuch merged commit f30c289 into envoyproxy:main Aug 27, 2021

adisuissa mentioned this pull request Apr 8, 2022

Re-enable buf for API breaking change detection #20746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Add API protobuf breaking-change-detector CI script #17793

ci: Add API protobuf breaking-change-detector CI script #17793

YaseenAlk commented Aug 20, 2021 •

edited

Loading

repokitteh-read-only bot commented Aug 20, 2021

YaseenAlk commented Aug 20, 2021

akonradi left a comment

akonradi Aug 20, 2021

YaseenAlk commented Aug 21, 2021

repokitteh-read-only bot commented Aug 21, 2021

YaseenAlk commented Aug 24, 2021

repokitteh-read-only bot commented Aug 24, 2021

adisuissa left a comment

adisuissa Aug 24, 2021

akonradi left a comment

htuch Aug 26, 2021

mattklein123 Aug 26, 2021

YaseenAlk Aug 27, 2021

akonradi Aug 27, 2021

YaseenAlk commented Aug 27, 2021

akonradi left a comment

akonradi Aug 27, 2021

adisuissa left a comment

adisuissa commented Aug 27, 2021

johanbrandhorst Aug 27, 2021

YaseenAlk commented Aug 27, 2021

johanbrandhorst Aug 27, 2021

YaseenAlk commented Aug 27, 2021

YaseenAlk commented Aug 27, 2021

repokitteh-read-only bot commented Aug 27, 2021

htuch left a comment

htuch commented Aug 27, 2021

htuch commented Aug 27, 2021

ci: Add API protobuf breaking-change-detector CI script #17793

ci: Add API protobuf breaking-change-detector CI script #17793

Conversation

YaseenAlk commented Aug 20, 2021 • edited Loading

repokitteh-read-only bot commented Aug 20, 2021

YaseenAlk commented Aug 20, 2021

akonradi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaseenAlk commented Aug 21, 2021

repokitteh-read-only bot commented Aug 21, 2021

YaseenAlk commented Aug 24, 2021

repokitteh-read-only bot commented Aug 24, 2021

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akonradi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaseenAlk commented Aug 27, 2021

akonradi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa commented Aug 27, 2021

Choose a reason for hiding this comment

YaseenAlk commented Aug 27, 2021

Choose a reason for hiding this comment

YaseenAlk commented Aug 27, 2021

YaseenAlk commented Aug 27, 2021

repokitteh-read-only bot commented Aug 27, 2021

htuch left a comment

Choose a reason for hiding this comment

htuch commented Aug 27, 2021

htuch commented Aug 27, 2021

YaseenAlk commented Aug 20, 2021 •

edited

Loading