Skip to content
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

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

Merged
merged 16 commits into from
Aug 27, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,11 @@ proto_library(
":v3_protos",
],
)

filegroup(
name = "proto_breaking_change_detector_buf_config",
srcs = [
"buf.yaml",
],
visibility = ["//visibility:public"],
)
6 changes: 3 additions & 3 deletions api/bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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.48.2",
sha256 = "ee0ea6c4a7bbb016d79b056905c0a1f018e7c5e47b37038c993a77b1bc732c0d",
version = "0.52.0",
sha256 = "db7f5ad462b8e8b9710cd15f5b239015cb7af6b8eaed3e490ec6881b43c7c7d6",
strip_prefix = "buf",
urls = ["https://github.com/bufbuild/buf/releases/download/v{version}/buf-Linux-x86_64.tar.gz"],
release_date = "2021-07-30",
release_date = "2021-08-19",
use_category = ["api"],
tags = ["manual"],
),
Expand Down
52 changes: 52 additions & 0 deletions api/buf.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Generated by buf. DO NOT EDIT.
version: v1
deps:
- remote: buf.build
owner: beta
repository: gogo
branch: main
commit: 2b52cca11cf242b09346114eeda81384
digest: b1-3_owLgnpQ3S9GCClrVq3aBuqFMx0yZ5N545nF0SiyS4=
create_time: 2021-02-09T19:58:24.352989Z
- remote: buf.build
owner: beta
repository: googleapis
branch: main
commit: ac0d624de3f746c3b8ef55c74b6f5244
digest: b1-GubXb4hZ56wTohZYDL-Muute9Jxx8J4TtIYucS30TBM=
create_time: 2021-08-12T19:53:55.128813Z
- 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: 549da630ffe24b53be3983fcee3bb346
digest: b1-HVAvWKH61BF6TdZSbHRhrD2SUuC0V7uAlZgCRimGPLI=
create_time: 2021-08-09T14:24:57.923964Z
- 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: 82388a0a0cb04e98a203f95dfed5e84b
digest: b1-lYgUMN58PxyCwvfQoopp40AJ-oHHjWXAzksF7v9U-U4=
create_time: 2021-06-21T22:00:30.152545Z
- remote: buf.build
owner: beta
repository: xds
branch: main
commit: 45f850b92541434cbde4aece01bc7d53
digest: b1-QZUL5DC6-nVgMMlajH_hlImwghg5HjRsqlEAOl0dZgI=
create_time: 2021-08-09T14:37:06.872899Z
10 changes: 8 additions & 2 deletions ...i_proto_breaking_change_detector/buf.yaml → api/buf.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
version: v1beta1
deps:
- buf.build/beta/googleapis

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- buf.build/beta/opencensus
- buf.build/beta/prometheus
- buf.build/beta/opentelemetry
- buf.build/beta/gogo
- buf.build/beta/xds
breaking:
ignore_unstable_packages: true
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
1 change: 1 addition & 0 deletions bazel/api_binding.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def _default_envoy_api_impl(ctx):
"tools",
"versioning",
"contrib",
"buf.yaml",
]
for d in api_dirs:
ctx.symlink(ctx.path(ctx.attr.envoy_root).dirname.get_child(ctx.attr.reldir).get_child(d), d)
Expand Down
10 changes: 10 additions & 0 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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..."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure SGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bazel build @com_github_bufbuild_buf//:buf
BUF_PATH=$(realpath "bazel-source/external/com_github_bufbuild_buf/bin/buf")
echo "Checking API for breaking changes to protobuf backwards compatibility..."
BASE_BRANCH_REF=$("${ENVOY_SRCDIR}"/tools/git/last_github_commit.sh)
COMMIT_TITLE=$(git log -n 1 --pretty='format:%C(auto)%h (%s, %ad)' "${BASE_BRANCH_REF}")
echo -e "\tUsing base commit ${COMMIT_TITLE}"
# TODO: remove "|| true". currently added so that this step always exits with code 0
# we want to roll this out as an "advisory" CI step, without interrupting the PR process
"${ENVOY_SRCDIR}"/tools/api_proto_breaking_change_detector/detector_ci.sh "${BUF_PATH}" "${BASE_BRANCH_REF}" || true
exit 0
elif [[ "$CI_TARGET" == "bazel.coverage" || "$CI_TARGET" == "bazel.fuzz_coverage" ]]; then
setup_clang_toolchain
Expand Down
8 changes: 8 additions & 0 deletions generated_api_shadow/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,11 @@ proto_library(
":v3_protos",
],
)

filegroup(
name = "proto_breaking_change_detector_buf_config",
srcs = [
"buf.yaml",
],
visibility = ["//visibility:public"],
)
6 changes: 3 additions & 3 deletions generated_api_shadow/bazel/repository_locations.bzl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 25 additions & 5 deletions tools/api_proto_breaking_change_detector/BUILD
Original file line number Diff line number Diff line change
@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also have detector_ci.py here.

name = "proto_breaking_change_detector",
srcs = ["detector.py"],
srcs = [
"detector.py",
],
data = [
":buf.lock",
":buf.yaml",
"@com_github_bufbuild_buf//:buf",
"@envoy_api_canonical//:proto_breaking_change_detector_buf_config",
],
main = "detector.py",
tags = ["manual"],
deps = [
":buf_utils",
":proto_breaking_change_detector_errors",
"//tools:run_command",
],
)

py_library(
name = "buf_utils",
srcs = [
"buf_utils.py",
],
deps = [
":proto_breaking_change_detector_errors",
"//tools/base:utils",
],
)

py_library(
name = "proto_breaking_change_detector_errors",
YaseenAlk marked this conversation as resolved.
Show resolved Hide resolved
srcs = [
"detector_errors.py",
],
)

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",
Expand Down
45 changes: 0 additions & 45 deletions tools/api_proto_breaking_change_detector/buf.lock

This file was deleted.

Loading