diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index b0fd72d44f..5bcc5e1ed4 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -19,7 +19,11 @@ concurrency: # Auto-cancel existing runs in the PR when a new commit is pushed cancel-in-progress: true env: - COMMIT: ${{ github.event.inputs.commit || github.sha }} + # For workflow_dispatch: use the given commit. + # For pull_request: use the head of the PR branch (not the merge branch which is the default!) + # For push: use the pushed commit. + COMMIT: ${{ github.event.inputs.commit || github.event.pull_request.head.sha || github.sha }} + PR_BASE_COMMIT: ${{ github.event.pull_request.base.sha }} DOCKER_COMPOSE_FILE: ./develop/github/docker-compose.yml TEMPORAL_VERSION_CHECK_DISABLED: 1 BUILDKITE_ANALYTICS_TOKEN: ${{ secrets.BUILDKITE_ANALYTICS_TOKEN }} @@ -37,7 +41,8 @@ jobs: with: token: ${{ secrets.GITHUB_TOKEN }} ref: ${{ env.COMMIT }} - submodules: true + # buf-breaking tries to compare HEAD against merge base so we need to be able to find it + fetch-depth: 100 - uses: actions/setup-go@v5 with: diff --git a/.gitignore b/.gitignore index 823821376c..e8778dd55e 100644 --- a/.gitignore +++ b/.gitignore @@ -24,7 +24,7 @@ /tctl* /tdbg -# Buf proto image +# proto image /proto/image.bin # api+google proto dependencies /proto/api.binpb diff --git a/Makefile b/Makefile index 249c18fdb6..761a3eaef2 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,7 @@ bins: temporal-server temporal-cassandra-tool temporal-sql-tool tdbg all: clean proto bins check test # Used in CI -ci-build-misc: print-go-version proto bins temporal-server-debug shell-check copyright-check go-generate gomodtidy ensure-no-changes +ci-build-misc: print-go-version proto buf-breaking bins temporal-server-debug shell-check copyright-check go-generate gomodtidy ensure-no-changes # Delete all build artifacts clean: clean-bins clean-test-results @@ -70,8 +70,10 @@ PROTO_ROOT := proto PROTO_FILES = $(shell find ./$(PROTO_ROOT)/internal -name "*.proto") PROTO_DIRS = $(sort $(dir $(PROTO_FILES))) API_BINPB := $(PROTO_ROOT)/api.binpb +# Note: If you change the value of INTERNAL_BINPB, you'll have to add logic to +# develop/buf-breaking.sh to handle the old and new values at once. +INTERNAL_BINPB := $(PROTO_ROOT)/image.bin PROTO_OUT := api -PROTO_ENUMS := $(shell grep -R '^enum ' $(PROTO_ROOT) | cut -d ' ' -f2) ALL_SRC := $(shell find . -name "*.go") ALL_SRC += go.mod @@ -228,13 +230,17 @@ endef ##### Proto ##### $(API_BINPB): go.mod go.sum $(PROTO_FILES) - @printf $(COLOR) "Generate api.binpb..." + @printf $(COLOR) "Generating proto dependencies image..." @./cmd/tools/getproto/run.sh --out $@ +$(INTERNAL_BINPB): $(API_BINPB) $(PROTO_FILES) + @printf $(COLOR) "Generate proto image..." + @protoc --descriptor_set_in=$(API_BINPB) -I=$(PROTO_ROOT)/internal $(PROTO_FILES) -o $@ + protoc: $(PROTOGEN) $(MOCKGEN) $(GOIMPORTS) $(PROTOC_GEN_GO) $(PROTOC_GEN_GO_GRPC) $(PROTOC_GEN_GO_HELPERS) $(API_BINPB) @env \ PROTOGEN=$(PROTOGEN) MOCKGEN=$(MOCKGEN) GOIMPORTS=$(GOIMPORTS) \ - API_BINPB=$(API_BINPB) PROTO_OUT=$(PROTO_OUT) \ + API_BINPB=$(API_BINPB) PROTO_ROOT=$(PROTO_ROOT) PROTO_OUT=$(PROTO_OUT) \ ./develop/protoc.sh service-clients: @@ -283,13 +289,13 @@ copyright: @printf $(COLOR) "Fix license header..." @go run ./cmd/tools/copyright/licensegen.go -goimports: MERGE_BASE ?= $(shell test -d .git && git merge-base $(MAIN_BRANCH) HEAD) -goimports: MODIFIED_FILES := $(shell test -d .git && git diff --name-status $(MERGE_BASE) -- | cut -f2) goimports: @printf $(COLOR) "Run goimports for modified files..." - @printf "Merge base: $(MERGE_BASE)\n" - @printf "Modified files: $(MODIFIED_FILES)\n" - @$(GOIMPORTS_BIN) -w $(filter %.go, $(MODIFIED_FILES)) + @MERGE_BASE=$$(git merge-base $(MAIN_BRANCH) HEAD) && \ + MODIFIED_FILES=$$(git diff --name-status $$MERGE_BASE -- | cut -f2 | grep '.go$$' || true) && \ + echo "Merge base: $$MERGE_BASE" && \ + echo "Modified files: $$MODIFIED_FILES" && \ + if [ -n "$$MODIFIED_FILES" ]; then $(GOIMPORTS) -w $$MODIFIED_FILES; fi lint-actions: $(ACTIONLINT) @printf $(COLOR) "Linting GitHub actions..." @@ -306,18 +312,14 @@ lint-api: $(API_LINTER) $(API_BINPB) @printf $(COLOR) "Linting proto API..." $(call silent_exec, $(API_LINTER) --set-exit-status -I=$(PROTO_ROOT)/internal --descriptor-set-in $(API_BINPB) --config=$(PROTO_ROOT)/api-linter.yaml $(PROTO_FILES)) -lint-protos: $(BUF) $(API_BINPB) +lint-protos: $(BUF) $(INTERNAL_BINPB) @printf $(COLOR) "Linting proto definitions..." - @protoc --descriptor_set_in=$(API_BINPB) -I=proto/internal $(PROTO_FILES) -o /dev/stdout | (cd proto/internal && $(ROOT)/$(BUF) lint -) + @$(BUF) lint $(INTERNAL_BINPB) -# TODO: fix this to work with getproto + API_BINPB -# buf-build: $(BUF) -# @printf $(COLOR) "Build image.bin with buf..." -# @(cd $(PROTO_ROOT) && $(ROOT)/$(BUF) build -o image.bin) -# -# buf-breaking: $(BUF) -# @printf $(COLOR) "Run buf breaking changes check against image.bin..." -# @(cd $(PROTO_ROOT) && $(ROOT)/$(BUF) breaking --against image.bin) +buf-breaking: $(BUF) $(API_BINPB) $(INTERNAL_BINPB) + @printf $(COLOR) "Run buf breaking proto changes check..." + @env BUF=$(BUF) API_BINPB=$(API_BINPB) INTERNAL_BINPB=$(INTERNAL_BINPB) MAIN_BRANCH=$(MAIN_BRANCH) \ + ./develop/buf-breaking.sh shell-check: @printf $(COLOR) "Run shellcheck for script files..." diff --git a/develop/buf-breaking.sh b/develop/buf-breaking.sh new file mode 100755 index 0000000000..2299693c0b --- /dev/null +++ b/develop/buf-breaking.sh @@ -0,0 +1,86 @@ +#!/usr/bin/env bash +# +# This script is intended to be run from the Makefile by the `buf-breaking` target. +# +# This uses the buf tool (https://buf.build/) to check our proto definitions for invalid +# changes. Because of limitations of buf (it doesn't support using images for +# dependencies), we use it by building our protos into images and then calling buf to +# compare images, instead of using buf's built-in git support. That means we have to use +# git manually to collect the versions we want to compare. +# +# We run two breaking checks: this commit against its PR merge base, and also this commit +# against the current head of main. Checking against the merge base is the one that will +# find most breakage, but it doesn't handle the case where protos are evolved in +# compatible ways in different directions on a feature branch vs the main branch, that are +# not compatible with each other. That case would hopefully be detected at merge time, but +# it's better to detect it early during feature branch development. The check against main +# handles that. + +set -eu -o pipefail + +# These are passed in by the Makefile: +: "${MAKE:=make}" +: "${BUF:=buf}" +: "${API_BINPB:=proto/api.binpb}" +: "${INTERNAL_BINPB:=proto/image.bin}" +: "${MAIN_BRANCH:=main}" + +color() { printf "\e[1;35m%s\e[0m\n" "$*" ; } +yellow() { printf "\e[1;33m%s\e[0m\n" "$*" ; } +red() { printf "\e[1;31m%s\e[0m\n" "$*" ; } + +if [[ -n "$(git status --porcelain --untracked-files=no)" ]]; then + red "Commit all local changes before running buf-breaking" + git status + # Exit with success here. + # Interactively: the user will see this and know what to do. In CI: this is run as + # part of ci-build-misc, which has a "ensure-no-changes at the end". Fail there + # instead of here since we can run more tests and the error message is nicer. + exit +fi + +# If invoked from the Makefile, this should already be done. This is just in +# case this is being run manually. +$MAKE "$INTERNAL_BINPB" + +tmp=$(mktemp --tmpdir -d temporal-buf-breaking.XXXXXXXXX) +trap 'rm -rf $tmp' EXIT + +check_against_commit() { + local commit=$1 name=$2 + color "Breaking check against $name:" + git -C "$tmp" checkout --detach "$commit" + # Note that we're now in a different commit of this repo, so we're relying on different + # versions of the Makefile to do what we expect given this make target. Check that it's + # least new enough to handle this by looking for the string INTERNAL_BINPB. + if grep -q INTERNAL_BINPB "$tmp/Makefile"; then + $MAKE -C "$tmp" "$INTERNAL_BINPB" + $BUF breaking "$INTERNAL_BINPB" --against "$tmp/$INTERNAL_BINPB" --config proto/internal/buf.yaml + else + yellow "$name commit is too old to support breaking check" + fi +} + +if [[ $PR_BASE_COMMIT ]]; then + # We're running in GHA, using shallow clone. Fetch some commits from the PR + # base so we can try to find the merge base. + color "Fetching more commits from $PR_BASE_COMMIT..." + git fetch --no-tags --no-recurse-submodules --depth=100 origin "$PR_BASE_COMMIT" +fi + +color "Cloning repo to temp dir..." +git clone . "$tmp" + +if [[ $PR_BASE_COMMIT ]]; then + # First check against merge base commit: + git -C "$tmp" fetch origin "$PR_BASE_COMMIT" + if base=$(git -C "$tmp" merge-base HEAD "$PR_BASE_COMMIT"); then + check_against_commit "$base" "merge base" + else + yellow "Can't find merge base for breaking check, checking against main only" + fi +fi + +# Next check against main: +git -C "$tmp" fetch --depth=1 https://github.com/temporalio/temporal.git "$MAIN_BRANCH" +check_against_commit FETCH_HEAD "main" diff --git a/develop/protoc.sh b/develop/protoc.sh index e355713451..db61289d2f 100755 --- a/develop/protoc.sh +++ b/develop/protoc.sh @@ -13,7 +13,7 @@ rm -rf "$new" && mkdir -p "$new" $PROTOGEN \ --descriptor_set_in="$API_BINPB" \ - --root=proto/internal \ + --root="$PROTO_ROOT"/internal \ --rewrite-enum=BuildId_State:BuildId \ --output="$new" \ -p go-grpc_out=paths=source_relative:"$new" \ diff --git a/proto/internal/buf.yaml b/proto/internal/buf.yaml index c1078f6504..3622df1e05 100644 --- a/proto/internal/buf.yaml +++ b/proto/internal/buf.yaml @@ -9,6 +9,9 @@ deps: breaking: use: - WIRE + # Uncomment this to temporarily ignore specific files or directories: + #ignore: + # - temporal/server/api/... lint: use: - DEFAULT