From cb6353f9afe1cf920d10ab1c9fce2eb9353de2a2 Mon Sep 17 00:00:00 2001 From: Piotr Zaniewski Date: Wed, 3 Jun 2026 09:08:59 +0200 Subject: [PATCH] fix(ci-test-notify): skip slack alert for cancelled and skipped runs Cancelled GitHub Actions runs were posting Slack alerts as if they had failed. Callers wire the run conclusion straight into this shared action via needs..result or job.status, which yields success, failure, cancelled, or skipped. The action sent a message for any of them, so a human cancelling a run (or a superseded/skipped job) produced noise in the CI alerts channel indistinguishable from a real failure. Gate the send in the shared action rather than in every caller: a new should-notify.sh resolves cancelled and skipped to a no-op (only success and failure notify) and folds in the existing empty-webhook check for fork PRs. Fixing it here protects all callers (nightly E2E, vcluster-pro conformance) without each repeating an if: guard. Covered by should-notify.bats (10 cases); make test-ci-test-notify now runs the whole test dir so new bats files are picked up locally too. Closes DEVOPS-960 --- .github/actions/ci-test-notify/README.md | 13 ++- .github/actions/ci-test-notify/action.yml | 16 ++-- .../actions/ci-test-notify/should-notify.sh | 29 ++++++ .../ci-test-notify/test/should-notify.bats | 94 +++++++++++++++++++ Makefile | 2 +- 5 files changed, 145 insertions(+), 9 deletions(-) create mode 100755 .github/actions/ci-test-notify/should-notify.sh create mode 100644 .github/actions/ci-test-notify/test/should-notify.bats diff --git a/.github/actions/ci-test-notify/README.md b/.github/actions/ci-test-notify/README.md index 38875bc..da88580 100644 --- a/.github/actions/ci-test-notify/README.md +++ b/.github/actions/ci-test-notify/README.md @@ -11,7 +11,7 @@ Replaces the nightly-specific `ci-notify-nightly-tests` action with a generic in | INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | |-------------|--------|----------|---------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | details | string | false | | Markdown text appended after the build
URL (test results, versions, artifact links, etc.) | -| status | string | true | | Test status: success, failure, cancelled, or
skipped | +| status | string | true | | Run status, typically `needs..result` or `job.status`.
`success` and `failure` notify; `cancelled` and
`skipped` are treated as no-ops and
send nothing. | | test-name | string | true | | Test suite name for the header
(e.g. "E2E Ginkgo Nightly Tests"). Keep under ~130 chars —
Slack header blocks have a 150-char
limit and the status suffix takes
~15 chars. | | webhook-url | string | true | | Slack incoming webhook URL | @@ -73,6 +73,17 @@ Build URL: webhook-url: ${{ secrets.SLACK_WEBHOOK_URL_DEV_VCLUSTER }} ``` +## Notification gating + +The action only notifies on actionable outcomes. A `status` of `cancelled` or +`skipped` is treated as a no-op: the action logs a notice and sends nothing. +This means callers can pass `needs..result` or `job.status` straight +through without a guard. A cancelled run (aborted by a human or superseded) or a +skipped job never produces a Slack alert; only `success` and `failure` do. + +An empty `webhook-url` (fork PRs, where secrets are unavailable) also suppresses +the notification. + ## Permissions No special GitHub permissions required. The `webhook-url` must be supplied via a repository secret. diff --git a/.github/actions/ci-test-notify/action.yml b/.github/actions/ci-test-notify/action.yml index be4f4f2..4f40dd6 100644 --- a/.github/actions/ci-test-notify/action.yml +++ b/.github/actions/ci-test-notify/action.yml @@ -8,7 +8,7 @@ inputs: description: 'Test suite name for the header (e.g. "E2E Ginkgo Nightly Tests"). Keep under ~130 chars — Slack header blocks have a 150-char limit and the status suffix takes ~15 chars.' required: true status: - description: 'Test status: success, failure, cancelled, or skipped' + description: 'Run status, typically `needs..result` or `job.status`. `success` and `failure` notify; `cancelled` and `skipped` are treated as no-ops and send nothing.' required: true details: description: 'Markdown text appended after the build URL (test results, versions, artifact links, etc.)' @@ -21,14 +21,16 @@ inputs: runs: using: "composite" steps: - - name: Check webhook URL - if: inputs.webhook-url == '' + - name: Decide whether to notify + id: gate shell: bash - run: | - echo "::warning::webhook-url is empty (expected on fork PRs where secrets are unavailable), skipping notification" + env: + STATUS: ${{ inputs.status }} + WEBHOOK_URL: ${{ inputs.webhook-url }} + run: ${{ github.action_path }}/should-notify.sh - name: Build Slack payload - if: inputs.webhook-url != '' + if: steps.gate.outputs.notify == 'true' shell: bash env: TEST_NAME: ${{ inputs.test-name }} @@ -41,7 +43,7 @@ runs: run: ${{ github.action_path }}/build-payload.sh - name: Send Slack notification - if: inputs.webhook-url != '' + if: steps.gate.outputs.notify == 'true' uses: slackapi/slack-github-action@45a88b9581bfab2566dc881e2cd66d334e621e2c # v3.0.3 with: errors: true diff --git a/.github/actions/ci-test-notify/should-notify.sh b/.github/actions/ci-test-notify/should-notify.sh new file mode 100755 index 0000000..a229af8 --- /dev/null +++ b/.github/actions/ci-test-notify/should-notify.sh @@ -0,0 +1,29 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Decides whether ci-test-notify should send a Slack message, writing +# `notify=true|false` to $GITHUB_OUTPUT for the composite action to gate on. +# +# Callers pass the run conclusion straight from `needs..result` or +# `job.status`, which can be success, failure, cancelled, or skipped. Only +# success and failure are actionable: a cancelled run was aborted by a human +# (or superseded), and a skipped job never executed. Neither warrants a Slack +# alert, so both are silenced here rather than in every caller. +# +# An empty webhook (fork PRs, where secrets are unavailable) also suppresses +# the notification, same as before. +# +# Required env vars: STATUS, GITHUB_OUTPUT +# Optional env vars: WEBHOOK_URL + +notify=true + +if [[ -z "${WEBHOOK_URL:-}" ]]; then + echo "::warning::webhook-url is empty (expected on fork PRs where secrets are unavailable), skipping notification" + notify=false +elif [[ "${STATUS:?STATUS is required}" == "cancelled" || "$STATUS" == "skipped" ]]; then + echo "::notice::status is '$STATUS' — only success and failure notify, skipping Slack notification" + notify=false +fi + +echo "notify=$notify" >> "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" diff --git a/.github/actions/ci-test-notify/test/should-notify.bats b/.github/actions/ci-test-notify/test/should-notify.bats new file mode 100644 index 0000000..a306861 --- /dev/null +++ b/.github/actions/ci-test-notify/test/should-notify.bats @@ -0,0 +1,94 @@ +#!/usr/bin/env bats +# Tests for should-notify.sh — the gate that decides whether a Slack +# notification is sent. Cancelled and skipped runs must stay silent. + +SCRIPT="$BATS_TEST_DIRNAME/../should-notify.sh" + +setup() { + MOCK_DIR=$(mktemp -d) + export GITHUB_OUTPUT="$MOCK_DIR/output" + : > "$GITHUB_OUTPUT" + + # Sensible defaults; individual tests override STATUS / WEBHOOK_URL. + export STATUS="success" + export WEBHOOK_URL="https://hooks.slack.com/services/T000/B000/xxx" +} + +teardown() { + rm -rf "$MOCK_DIR" +} + +# Helper: read the notify value written to GITHUB_OUTPUT +notify_value() { + sed -n 's/^notify=//p' "$GITHUB_OUTPUT" +} + +# --- Statuses that should notify --- + +@test "success notifies" { + STATUS="success" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "true" ] +} + +@test "failure notifies" { + STATUS="failure" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "true" ] +} + +# --- Statuses that must stay silent (the bug this fixes) --- + +@test "cancelled does not notify" { + STATUS="cancelled" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "false" ] +} + +@test "skipped does not notify" { + STATUS="skipped" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "false" ] +} + +@test "cancelled emits an explanatory notice" { + STATUS="cancelled" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"::notice::"* ]] + [[ "$output" == *"cancelled"* ]] +} + +# --- Empty webhook (fork PRs) wins over an otherwise-notifying status --- + +@test "empty webhook does not notify even on failure" { + STATUS="failure" WEBHOOK_URL="" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "false" ] + [[ "$output" == *"webhook-url is empty"* ]] +} + +@test "empty webhook does not notify on success" { + STATUS="success" WEBHOOK_URL="" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "false" ] +} + +# --- Error handling --- + +@test "fails when STATUS is unset" { + unset STATUS + run bash "$SCRIPT" + [ "$status" -ne 0 ] +} + +@test "fails when GITHUB_OUTPUT is unset" { + unset GITHUB_OUTPUT + run bash "$SCRIPT" + [ "$status" -ne 0 ] +} + +@test "exactly one notify line is written" { + STATUS="success" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(grep -c '^notify=' "$GITHUB_OUTPUT")" -eq 1 ] +} diff --git a/Makefile b/Makefile index 9717582..2e7f060 100644 --- a/Makefile +++ b/Makefile @@ -123,7 +123,7 @@ test-ai-step: ## run ai-step bats tests bats $(ACTIONS_DIR)/ai-step/test/*.bats test-ci-test-notify: ## run ci-test-notify bats tests - bats $(ACTIONS_DIR)/ci-test-notify/test/build-payload.bats + bats $(ACTIONS_DIR)/ci-test-notify/test test-publish-helm-chart: ## run publish-helm-chart bats tests (requires mikefarah/yq on PATH) bats $(ACTIONS_DIR)/publish-helm-chart/test/run.bats