From 1a6a200403019072042469bcd52b9612e4c5bda7 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Mon, 24 Jun 2024 06:06:52 -0600 Subject: [PATCH] CI: logformatter: link to correct PR base Two enormous misunderstandings: 1) $CIRRUS_BASE_SHA is worthless. I thought it was, you know, the BASE SHA of the current commit, but (as best I can tell) it seems to be the SHA of the most recent commit on the destination branch. Cirrus docs are unhelpful. Anyhow, it's clearly not anything useful. Stop using it. 2) $EPOCH_TEST_COMMIT is closer to what we want. It is defined in Makefile as the git merge-base. But for unknown reasons it was being clobbered in CI scripts, and it doesn't seem to work in all contexts, so, eliminate it from CI setup scripts. Leave it only in Makefile. This leaves us with no option other than defining our own merge-base variable, PR_BASE_SHA. Do so and pass it along to rootless jobs. Signed-off-by: Ed Santiago --- Makefile | 1 - contrib/cirrus/lib.sh | 22 ++++++---------------- contrib/cirrus/logformatter | 11 +++++++---- contrib/cirrus/runner.sh | 16 +++++++--------- 4 files changed, 20 insertions(+), 30 deletions(-) diff --git a/Makefile b/Makefile index 4fe8f8a353..90870b5771 100644 --- a/Makefile +++ b/Makefile @@ -276,7 +276,6 @@ help: ## (Default) Print listing of key targets with their descriptions .PHONY: lint lint: golangci-lint - @echo "Linting vs commit '$(call err_if_empty,EPOCH_TEST_COMMIT)'" ifeq ($(PRE_COMMIT),) @echo "FATAL: pre-commit was not found, make .install.pre-commit to installing it." >&2 @exit 2 diff --git a/contrib/cirrus/lib.sh b/contrib/cirrus/lib.sh index ac4b53af1e..d47b310532 100644 --- a/contrib/cirrus/lib.sh +++ b/contrib/cirrus/lib.sh @@ -71,30 +71,20 @@ export CI="${CI:-false}" CIRRUS_CI="${CIRRUS_CI:-false}" CONTINUOUS_INTEGRATION="${CONTINUOUS_INTEGRATION:-false}" CIRRUS_REPO_NAME=${CIRRUS_REPO_NAME:-podman} -# Cirrus only sets $CIRRUS_BASE_SHA properly for PRs, but $EPOCH_TEST_COMMIT -# needs to be set from this value in order for `make validate` to run properly. -# When running get_ci_vm.sh, most $CIRRUS_xyz variables are empty. Attempt -# to accommodate both branch and get_ci_vm.sh testing by discovering the base -# branch SHA value. + # shellcheck disable=SC2154 -if [[ -z "$CIRRUS_BASE_SHA" ]] && [[ -z "$CIRRUS_TAG" ]] -then # Operating on a branch, or under `get_ci_vm.sh` - showrun echo "branch or get_ci_vm (CIRRUS_BASE_SHA and CIRRUS_TAG are unset)" - CIRRUS_BASE_SHA=$(git rev-parse ${UPSTREAM_REMOTE:-origin}/$DEST_BRANCH) -elif [[ -z "$CIRRUS_BASE_SHA" ]] -then # Operating on a tag - showrun echo "operating on tag" - CIRRUS_BASE_SHA=$(git rev-parse HEAD) +if [[ -n "$CIRRUS_PR" ]] && [[ -z "$PR_BASE_SHA" ]]; then + # shellcheck disable=SC2154 + PR_BASE_SHA=$(git merge-base ${DEST_BRANCH:-main} HEAD) + export PR_BASE_SHA fi -# The starting place for linting and code validation -EPOCH_TEST_COMMIT="$CIRRUS_BASE_SHA" # The next three values define regular expressions matching env. vars. necessary # for all possible testing contexts (rootless, container, etc.). These values # are consumed by the passthrough_envars() automation library function. # # List of envariables which must be EXACT matches -PASSTHROUGH_ENV_EXACT='CGROUP_MANAGER|DEST_BRANCH|DISTRO_NV|GOCACHE|GOPATH|GOSRC|NETWORK_BACKEND|OCI_RUNTIME|ROOTLESS_USER|SCRIPT_BASE|SKIP_USERNS|EC2_INST_TYPE|PODMAN_DB|STORAGE_FS|PODMAN_BATS_LEAK_CHECK' +PASSTHROUGH_ENV_EXACT='CGROUP_MANAGER|DEST_BRANCH|DISTRO_NV|GOCACHE|GOPATH|GOSRC|NETWORK_BACKEND|OCI_RUNTIME|PR_BASE_SHA|ROOTLESS_USER|SCRIPT_BASE|SKIP_USERNS|EC2_INST_TYPE|PODMAN_DB|STORAGE_FS|PODMAN_BATS_LEAK_CHECK' # List of envariable patterns which must match AT THE BEGINNING of the name. # Consumed by the passthrough_envars() automation library function. diff --git a/contrib/cirrus/logformatter b/contrib/cirrus/logformatter index a3b66cd20c..439c82f2fe 100755 --- a/contrib/cirrus/logformatter +++ b/contrib/cirrus/logformatter @@ -886,10 +886,13 @@ END_SYNOPSIS } $s .= _tr("Logs", join(" / ", @logs)); - # BASE_SHA can tell us if our parent includes--or doesn't--a purported - # fix for a flake. Note that "commits", plural, links to a git history - # listing; if we used "commit", singular, that would be less useful. - $s .= _tr("Base commit", _a("{CIRRUS_BASE_SHA}", "https://{CIRRUS_REPO_CLONE_HOST}/{CIRRUS_REPO_FULL_NAME}/commits/{CIRRUS_BASE_SHA}")); + # PR_BASE_SHA (set in lib.sh) can tell us if our parent includes--or + # doesn't--a purported fix for a flake. Note about the URL: "commits", plural, + # links to a git history listing; if we used "commit", singular, that would + # be less useful. + if (my $base = $ENV{PR_BASE_SHA}) { + $s .= _tr("Base commit", _a($base, "https://{CIRRUS_REPO_CLONE_HOST}/{CIRRUS_REPO_FULL_NAME}/commits/$base")); + } $s .= "\n"; return $s; diff --git a/contrib/cirrus/runner.sh b/contrib/cirrus/runner.sh index e9c0fa6bbd..550acc9f72 100755 --- a/contrib/cirrus/runner.sh +++ b/contrib/cirrus/runner.sh @@ -22,13 +22,8 @@ source $(dirname $0)/lib.sh showrun echo "starting" function _run_validate() { - # git-validation tool fails if $EPOCH_TEST_COMMIT is empty - # shellcheck disable=SC2154 - if [[ -n "$EPOCH_TEST_COMMIT" ]]; then - showrun make validate - else - warn "Skipping git-validation since \$EPOCH_TEST_COMMIT is empty" - fi + showrun make validate + # make sure PRs have tests showrun make tests-included } @@ -257,7 +252,8 @@ function _run_altbuild() { context_dir=$(mktemp -d --tmpdir make-size-check.XXXXXXX) savedhead=$(git rev-parse HEAD) # Push to PR base. First run of the script will write size files - pr_base=$(git merge-base origin/$DEST_BRANCH HEAD) + # shellcheck disable=SC2154 + pr_base=$PR_BASE_SHA showrun git checkout $pr_base showrun hack/make-and-check-size $context_dir # pop back to PR, and run incremental makes. Subsequent script @@ -457,7 +453,9 @@ function _bail_if_test_can_be_skipped() { # Defined by Cirrus-CI for all tasks # shellcheck disable=SC2154 head=$CIRRUS_CHANGE_IN_REPO - base=$(git merge-base $DEST_BRANCH $head) + # shellcheck disable=SC2154 + base=$PR_BASE_SHA + echo "_bail_if_test_can_be_skipped: head=$head base=$base" diffs=$(git diff --name-only $base $head) # If PR touches any files in an argument directory, we cannot skip