Skip to content

chore: add release Slack notifications#881

Merged
mbevc1 merged 7 commits into
mainfrom
20260511_release_notify
May 11, 2026
Merged

chore: add release Slack notifications#881
mbevc1 merged 7 commits into
mainfrom
20260511_release_notify

Conversation

@mbevc1
Copy link
Copy Markdown
Contributor

@mbevc1 mbevc1 commented May 11, 2026

  • Add notifications to Slack on CLI releases.
  • Removed old and not used slack-notification-on-failure (it was dead path of code - pushing a tag would never trigger merge to main)

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Claude finished @mbevc1's task in 2m 11s —— View job


PR Review: Add release Slack notifications

  • Run static analysis (go build, go vet, go mod tidy)
  • Read the current workflow changes
  • Review code quality, correctness, and security
  • Post review findings

Static Analysis

All clean — go build ./..., go vet ./..., and go mod tidy pass with no issues. (This PR only touches a workflow file, so this is expected.)

Review Summary

This PR replaces the old slack-notification-on-failure job with a proper start/finish notification system. The previous review comments have all been addressed in the latest commits:

Previous Issue Status
Bug: needs.release.result referencing non-existent job Fixed — now uses needs.*.result wildcard
needs list only covering 2 of 13 jobs Fixed — all 13 jobs listed in needs
Status check only gating on 2 jobs Fixed — uses !contains(join(needs.*.result, ','), 'failure') pattern
Missing harden-runner steps Fixed — both new jobs now include it
Extra fi causing shell syntax error Fixed — clean if/else/fi structure

What looks good

  • Clean replacement of broken code — the old slack-notification-on-failure had github.ref == 'refs/heads/main' in its condition, but this workflow only triggers on tag pushes (refs/tags/v*), so the old failure notification could never fire. This PR actually fixes a latent bug.
  • Proper dependency chainnotify-finish depends on all 13 pipeline jobs, and the always() + needs.notify-start.result == 'success' guard is well-designed: it ensures the finish notification runs regardless of job failures, but gracefully skips if the notification system itself is broken.
  • Status determination uses the same needs.*.result wildcard pattern that was already established in the workflow — consistent and correct.
  • Good permission scopingactions: read and contents: read are the minimum needed, with explanatory comments.

One nit (posted inline)

  • kosli-dev/reusable-actions/slack-release-notify@main is unpinned (tracks main branch), while every other action in this file is pinned to a commit SHA. Lower-risk for an internal repo, but pinning would be more consistent.

Verdict

The PR is in good shape after the iterative fixes. The core logic is correct and well-structured. The only suggestion is the minor consistency nit about SHA-pinning the internal action.

Comment thread .github/workflows/release.yml Outdated
Comment thread .github/workflows/release.yml Outdated
mbevc1 and others added 2 commits May 11, 2026 11:06
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Comment thread .github/workflows/release.yml Outdated
Comment thread .github/workflows/release.yml
mbevc1 and others added 2 commits May 11, 2026 11:21
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Comment thread .github/workflows/release.yml Outdated
Comment thread .github/workflows/release.yml
Comment thread .github/workflows/release.yml
Comment thread .github/workflows/release.yml
Comment thread .github/workflows/release.yml
@mbevc1 mbevc1 enabled auto-merge (squash) May 11, 2026 11:32
@mbevc1 mbevc1 merged commit 4c7daa9 into main May 11, 2026
20 checks passed
@mbevc1 mbevc1 deleted the 20260511_release_notify branch May 11, 2026 11:42
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.

2 participants