Skip to content

CLP-1040: Use common slack notification action - #545

Merged
frederic-tingaud-sonarsource merged 3 commits into
masterfrom
chore/mary-georgiou/CLP-1040-common-slack-notify
Oct 1, 2026
Merged

frederic-tingaud-sonarsource merged 3 commits into
masterfrom
chore/mary-georgiou/CLP-1040-common-slack-notify

Conversation

@mary-georgiou

@mary-georgiou mary-georgiou commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

CLP-1040: adopt the shared slack-notify composite action for build-failure notifications.

Adds a notify-build-failure job to build.yml (runs after build/promote/promote-nuget/promote-npm on failure for master, branch-*, dogfood-on-*), replacing the old draft's check_suite-triggered workflow, which is removed since that event never fires for GitHub Actions' own runs.

Notifies squad-corelang-notifs (channel from the previous draft, no prior working mechanism existed) with branch-patterns: 'master,branch-*,dogfood-on-*' to match this repo's push triggers.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

CLP-1040

Comment thread .github/workflows/slack-notify.yml Resolved
gitar-bot[bot]

This comment was marked as resolved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mary-georgiou
mary-georgiou force-pushed the chore/mary-georgiou/CLP-1040-common-slack-notify branch from 49bdd63 to faf53b1 Compare September 30, 2026 11:27
Comment on lines +361 to +365
needs: [ build, promote, promote-nuget, promote-npm ]
if: >-
always()
&& (github.ref_name == 'master' || startsWith(github.ref_name, 'branch-') || startsWith(github.ref_name, 'dogfood-on-'))
&& contains(needs.*.result, 'failure')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 [cross-pr] Notify job misses the scan job that PR #542 adds

This PR (head faf53b1) adds notify-build-failure with needs: [ build, promote, promote-nuget, promote-npm ], and it only fires when contains(needs.*.result, 'failure'). PR #542 (head 24f9c3d) moves Sonar analysis out of build into a new scan job and changes all three promote jobs to needs: [ build, scan ]. Suppose both PRs merge into build.yml on master, in either order, and nobody adds scan to the notifier's needs. Then a failure in scan alone leaves build green and the promote jobs skipped rather than failure. The contains check stays false and no Slack alert is sent, even though the master/branch pipeline failed.

Was this helpful? React with 👍 / 👎

@gitar-bot
gitar-bot Bot dismissed their stale review September 30, 2026 11:32

✅ All blocking issues resolved.

Configure merge blocking

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread .github/workflows/build.yml Resolved
@mary-georgiou
mary-georgiou marked this pull request as ready for review September 30, 2026 15:02
Co-authored-by: Mary Georgiou <89914005+mary-georgiou@users.noreply.github.com>
@sonarqube-next

Copy link
Copy Markdown

@frederic-tingaud-sonarsource
frederic-tingaud-sonarsource merged commit 0c23af2 into master Oct 1, 2026
9 checks passed
@frederic-tingaud-sonarsource
frederic-tingaud-sonarsource deleted the chore/mary-georgiou/CLP-1040-common-slack-notify branch October 1, 2026 16:21
@gitar-bot

gitar-bot Bot commented Oct 13, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 2 closed / 3 findings

🟡 Medium risk · Adds a failure-triggered CI notification job with job-scoped token permissions

Adopts the shared slack-notify action for build-failure notifications on master, branch-*, and dogfood-on-* branches, replacing the non-functional check_suite trigger. The notify-build-failure job's needs list should include the scan job added in PR #542 to catch scan-only failures; otherwise a scan failure alone will skip notification when other jobs remain green.

💡 [cross-pr] Notify job misses the scan job that PR #542 adds

📄 .github/workflows/build.yml:361-365

This PR (head faf53b1) adds notify-build-failure with needs: [ build, promote, promote-nuget, promote-npm ], and it only fires when contains(needs.*.result, 'failure'). PR #542 (head 24f9c3d) moves Sonar analysis out of build into a new scan job and changes all three promote jobs to needs: [ build, scan ]. Suppose both PRs merge into build.yml on master, in either order, and nobody adds scan to the notifier's needs. Then a failure in scan alone leaves build green and the promote jobs skipped rather than failure. The contains check stays false and no Slack alert is sent, even though the master/branch pipeline failed.

✅ 2 closed
✅ Bug: check_suite trigger never fires for this repo's GitHub Actions CI

📄 .github/workflows/slack-notify.yml:3-5 📄 .github/workflows/slack-notify.yml:20-21 🔗 link
All of this repo's CI runs on GitHub Actions (.github/workflows/build.yml, and there's no Cirrus or other external CI config). The GitHub docs say the check_suite event does not trigger workflows when "the check suite was created by GitHub Actions or if the check suite's head SHA is associated with GitHub Actions" (events that trigger workflows). That rule exists to stop workflows from triggering each other in a loop. So when the Build workflow fails on master, this workflow is never triggered, and no Slack notification is sent. The PR's main goal of adding failure notifications isn't met.

Fix: trigger on workflow_run for the Build workflow, filtered to the notified branches, and take the branch and conclusion from github.event.workflow_run.

✅ Quality: Job if still hardcodes 'master' while branch-patterns uses default_branch

📄 .github/workflows/build.yml:362-365 📄 .github/workflows/build.yml:376
This commit swaps the hardcoded master in branch-patterns for github.event.repository.default_branch, but the job's if gate on line 364 still checks github.ref_name == 'master'. The two filters now disagree. If the default branch is ever renamed (to main, say), the gate skips the job before the action runs, so default-branch failures stop producing notifications and the dynamic pattern never gets used. Build the gate from the same expression so both filters stay in sync.

🤖 Prompt for agents
Code Review: Adopts the shared `slack-notify` action for build-failure notifications on `master`, `branch-*`, and `dogfood-on-*` branches, replacing the non-functional `check_suite` trigger. The `notify-build-failure` job's `needs` list should include the `scan` job added in PR #542 to catch scan-only failures; otherwise a `scan` failure alone will skip notification when other jobs remain green.

1. 💡 [cross-pr] Notify job misses the `scan` job that PR [#542](<https://github.com/SonarSource/sonar-analyzer-commons/pull/542>) adds
   Files: .github/workflows/build.yml:361-365

   This PR (head faf53b17a56d305f2967bdcb1dd2f8265f92d83b) adds `notify-build-failure` with `needs: [ build, promote, promote-nuget, promote-npm ]`, and it only fires when `contains(needs.*.result, 'failure')`. PR https://github.com/SonarSource/sonar-analyzer-commons/pull/542 (head 24f9c3dbe0851e29a5ef26c6afe17a4142f09423) moves Sonar analysis out of `build` into a new `scan` job and changes all three promote jobs to `needs: [ build, scan ]`. Suppose both PRs merge into build.yml on master, in either order, and nobody adds `scan` to the notifier's `needs`. Then a failure in `scan` alone leaves `build` green and the promote jobs `skipped` rather than `failure`. The `contains` check stays false and no Slack alert is sent, even though the master/branch pipeline failed.

Verification

Functional validation No results

Rules No rules evaluated

Auto-approval Not enabled

Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants