Skip to content

feat(cicd): Add Slack message on Manual SDK Release#35461

Open
KevinDavilaDotCMS wants to merge 4 commits intomainfrom
feat/sdk-slack-notification-on-manual-release
Open

feat(cicd): Add Slack message on Manual SDK Release#35461
KevinDavilaDotCMS wants to merge 4 commits intomainfrom
feat/sdk-slack-notification-on-manual-release

Conversation

@KevinDavilaDotCMS
Copy link
Copy Markdown
Member

Here are examples of the types of messages


Auto-publish — next (PR merge)

▎ 🟠 Attention dotters: SDK libs (Angular, Client, Experiments and React) published!

▎ This automated script is happy to announce that a new SDK libs version tagged as: [ 1.5.1-next.1965 (next) ] is now available on the NPM registry 📦!
▎ Introduced by: #35412


Manual release — patch

▎ 🟢 Attention dotters: SDK libs (Angular, Client, Experiments and React) officially released!

▎ This automated script is happy to announce that a new SDK libs version tagged as: [ 1.5.2 (latest) and 1.5.2-next.1966 (next) ] is now available on the NPM registry 📦!
▎ Release type: patch | Triggered by: kevindavila
View workflow run


Manual release — minor

▎ 🟢 Attention dotters: SDK libs (Angular, Client, Experiments and React) officially released!

▎ This automated script is happy to announce that a new SDK libs version tagged as: [ 1.6.0 (latest) and 1.6.0-next.1970 (next) ] is now available on the NPM registry 📦!
▎ Release type: minor | Triggered by: kevindavila
View workflow run


Manual release — major

▎ 🟢 Attention dotters: SDK libs (Angular, Client, Experiments and React) officially released!

▎ This automated script is happy to announce that a new SDK libs version tagged as: [ 2.0.0 (latest) and 2.0.0-next.1975 (next) ] is now available on the NPM registry 📦!
▎ Release type: major | Triggered by: kevindavila
View workflow run


Manual release — custom

▎ 🟢 Attention dotters: SDK libs (Angular, Client, Experiments and React) officially released!

▎ This automated script is happy to announce that a new SDK libs version tagged as: [ 1.5.3 (latest) and 1.5.3-next.1980 (next) ] is now available on the NPM registry 📦!
▎ Release type: custom | Triggered by: kevindavila
View workflow run

… enrich auto-publish message

- Add missing Slack notification step to cicd_manual-release-sdks.yml; the manual
  release workflow called the deploy action directly but never sent a Slack message
- Include release type (patch/minor/major/custom) and triggering actor in the manual
  release notification, with a link to the workflow run
- Add PR extraction step to cicd_comp_deployment-phase.yml that parses the merge
  commit message for a PR number and falls back to the short commit SHA
- Enrich the auto-publish (next) Slack notification with an "Introduced by" link
  pointing to the PR or commit that triggered the SDK publish

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • This PR fixes #123 or Fixes: #123

  • This PR closes #123 or Closes: #123

  • This PR resolves #123 or Resolves: #123

  • Other supported keywords: fix, fixed, close, closed, resolve, resolved
    Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label Apr 26, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 26, 2026

Claude finished @KevinDavilaDotCMS's task in 1m 23s —— View job


PR Review

  • Read changed files and diff
  • Analyze for bugs, edge cases, security issues, design problems
  • Post findings

cicd_comp_deployment-phase.yml

1. Repository guard inconsistency
The existing Docker Slack notification has if: github.repository == 'dotcms/core', but the new SDK notification steps don't. They rely on SLACK_BOT_TOKEN being unavailable in forks (failing silently via continue-on-error: true). This works in practice but is inconsistent. The github.repository guard on the Docker step exists precisely to make intent explicit — the SDK steps should match.

# line 374 — current
if: success() && steps.sdks_publish.outputs.published == 'true'

# suggested
if: github.repository == 'dotcms/core' && steps.sdks_publish.outputs.published == 'true'

2. Shell interpolation style in "Extract PR" step
github.sha, github.server_url, and github.repository are interpolated directly into the shell script rather than passed via env: like COMMIT_MSG is. These values are predictably safe (hex SHA, fixed URL, dotcms/core) so there's no actual injection risk, but the inconsistency within the same step is surprising — a reader might wonder why COMMIT_MSG got the env treatment but these didn't.

# line 383-384 — mixes styles
echo "pr_url=${{ github.server_url }}/${{ github.repository }}/pull/$PR_NUMBER" >> $GITHUB_OUTPUT
echo "pr_ref=$(echo '${{ github.sha }}' | cut -c1-7)" >> $GITHUB_OUTPUT

3. success() is redundant (already noted in prior review, left as-is per author's comment)
Both if: success() && steps.sdks_publish.outputs.published == 'true' conditions — success() is the implicit default, so this is dead weight but harmless.


cicd_manual-release-sdks.yml

4. Slack payload doesn't match the PR description examples
The PR description shows the expected message as:

[ 1.5.2 (latest) and 1.5.2-next.1966 (next) ]

But the template only emits ${{ steps.deploy-javascript-sdk.outputs.npm-package-version }} — a single value. If the action's output only returns one version string, the notification won't match the mockup. Worth verifying what deploy-javascript-sdk actually outputs, or documenting that the PR description is aspirational.

5. Notification fires before post-release PR opens
The Slack step runs between "Publish @latest" and "Open post-release PR to bump VERSION on main". If the PR-opening step fails (e.g., branch already exists, network error), the Slack message has already announced the release successfully. This is arguably correct (the publish succeeded), but worth being intentional about — users will see the release announced even if the housekeeping PR didn't open.


Overall this is a narrow, low-risk addition. Issues 1 and 4 are the ones worth addressing before merge. The pipefail fix from the latest commit is correct.

…ations

- Move github.event.head_commit.message to env: block to prevent script
  injection (GitHub security best practice)
- Replace always() with success() on manual release Slack notification step
  for correct conditional semantics
- Add missing newline at end of cicd_comp_deployment-phase.yml

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@KevinDavilaDotCMS KevinDavilaDotCMS marked this pull request as ready for review April 27, 2026 17:32
@KevinDavilaDotCMS KevinDavilaDotCMS requested a review from a team as a code owner April 27, 2026 17:32
@KevinDavilaDotCMS KevinDavilaDotCMS linked an issue Apr 27, 2026 that may be closed by this pull request
1 task
…workflow

A Slack failure (bad token, network error, wrong channel) would skip the
post-release PR step, leaving the VERSION file stuck on the release version
on main and causing @next builds to conflict with @latest.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread .github/workflows/cicd_comp_deployment-phase.yml Outdated
… Slack step

- Add `|| true` to grep pipeline so a no-match exit code doesn't fail
  the step under bash pipefail, allowing the SHA fallback to run
- Add `continue-on-error: true` to deployment-phase SDK Slack notification
  to match the guard already present in the manual release workflow

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@KevinDavilaDotCMS
Copy link
Copy Markdown
Member Author

Addressed the two actionable items from the reviews in the latest commit (c1f0c50):

  • || true on grep pipeline — fixes the pipefail issue raised by @dcolina: a no-match exit code no longer kills the step, so the SHA fallback now works correctly (also covers empty COMMIT_MSG on workflow_dispatch callers).
  • continue-on-error: true on the deployment-phase SDK Slack step — now matches the guard already present in the manual release workflow.

The remaining claude[bot] observations (github.repository guard, success() redundancy, shell interpolation style) are noted but left as-is since they have no practical impact with the above fixes in place.

@dcolina
Copy link
Copy Markdown
Member

dcolina commented Apr 29, 2026

🔍 Platform Review — CI/CD

PR #35461 · feat/sdk-slack-notification-on-manual-releasemain · dotCMS/core · reviewed by Claude Code


Domains scanned

Domain Status
🟢 CI/CD (GitHub Actions) Reviewed
⚪ Terraform / IaC No changes
⚪ Kubernetes / Helm No changes
⚪ ArgoCD No changes

[🔵 Info] .github/workflows/cicd_comp_deployment-phase.yml:383-386 — Context expressions in run: not using env vars

${{ github.server_url }}, ${{ github.repository }}, and ${{ github.sha }} are interpolated directly inside the run: block of the Extract PR from commit message step — not via env vars.
These three values are not user-controlled (sha is always hex-only, server_url and repository are repo-owner-controlled), so there is no actual injection risk here. However, the pattern is inconsistent with the same step correctly handling COMMIT_MSG via an env var. Applying the same discipline to all context variables makes the safe/unsafe boundary explicit and makes future review easier.

# Current (mixes env-var and direct interpolation)
env:
  COMMIT_MSG: ${{ github.event.head_commit.message }}
run: |
  echo "pr_url=${{ github.server_url }}/${{ github.repository }}/pull/$PR_NUMBER" >> $GITHUB_OUTPUT

# Recommended (all context via env vars)
env:
  COMMIT_MSG: ${{ github.event.head_commit.message }}
  SERVER_URL: ${{ github.server_url }}
  REPOSITORY: ${{ github.repository }}
  COMMIT_SHA: ${{ github.sha }}
run: |
  echo "pr_url=${SERVER_URL}/${REPOSITORY}/pull/$PR_NUMBER" >> $GITHUB_OUTPUT

💡 Low priority — fix for consistency, not for security.


What looks good ✅

  • COMMIT_MSG: ${{ github.event.head_commit.message }} is correctly set as an env var before use in the run: block — the right pattern for potentially injection-capable event data.
  • continue-on-error: true on both Slack notification steps is correct — notification failures shouldn't block the release pipeline.
  • secrets.SLACK_BOT_TOKEN is passed via with:, never echoed in run: — no secret leakage.
  • ${{ inputs.version-type }} and ${{ github.actor }} used in with: on the composite action (not run:) — no script injection surface.
  • Local composite action ./.github/actions/core-cicd/notification/notify-slack — no SHA pinning needed for local refs.
  • No new permissions blocks introduced; existing trust boundary unchanged.

Legend — 🔴/🟠 fix before merge · 🟡 author triage · 🔵 advisory · updates on each push

@dcolina dcolina self-requested a review April 29, 2026 13:11
Copy link
Copy Markdown
Member

@dcolina dcolina left a comment

Choose a reason for hiding this comment

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

👍🏽

@KevinDavilaDotCMS KevinDavilaDotCMS added this pull request to the merge queue Apr 30, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Apr 30, 2026
@KevinDavilaDotCMS KevinDavilaDotCMS added this pull request to the merge queue Apr 30, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 30, 2026
@KevinDavilaDotCMS KevinDavilaDotCMS added this pull request to the merge queue Apr 30, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : CI/CD PR changes GitHub Actions/workflows

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Slack Message is doesn't triggered from Manual SDK Release

3 participants