Skip to content

slack notification update - #1326

Open
jth-nw wants to merge 2 commits into
devfrom
feat/slack-docs-gh-notifications
Open

slack notification update#1326
jth-nw wants to merge 2 commits into
devfrom
feat/slack-docs-gh-notifications

Conversation

@jth-nw

@jth-nw jth-nw commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Code Review

Reviewed the workflow + script changes only (no doc content). The pull_request_target hygiene is right — base-ref-only checkout, SHA-pinned actions, no head checkout, secrets never handed to PR code, and no leftover TEAMS_WEBHOOK_URL references after the sync-workflow cleanup. The problems below are all in the new label→notify chain, and several of them mean no Slack notification at all for common PRs.

1. label-pr.yml hard-fails on any PolicyPak / Endpoint Policy Manager PR (blocking)

.github/label-codeowners.json maps two labels to the same team:

"endpoint-policy-manager": "@netwrix/endpointpolicymanager-docs",
"policypak":               "@netwrix/endpointpolicymanager-docs",

scripts/resolve-labels-for-teams.mjs reverse-maps team → label by iterating every entry, so that team resolves to both labels: endpoint-policy-manager,policypak. But endpoint-policy-manager does not exist as a repo label (gh label list has only policypak).

gh pr edit --add-label validates label names against the repo and errors on an unknown one ('endpoint-policy-manager' not found) — it does not create it. So for any PR touching docs/policypak/, sidebars/policypak.js, or static/{files,images}/policypak/:

  • label-pr.yml line 60 exits non-zero → the workflow run is red,
  • no labels are applied (the call is atomic, so policypak isn't applied either),
  • no labeled event fires → slack-notify-pr.yml never runs → no Slack notification and no fork codeowners comment.

Fix: drop the stale endpoint-policy-manager key from the mapping (or make the reverse map pick one canonical label per team), and consider per-label adds so one bad name can't swallow the rest.

2. Same hard failure for Activity Monitor PRs (blocking)

The mapping key is "activity monitor" (space), but the repo label is activity-monitor (hyphen). gh pr edit --add-label "activity monitor" fails the same way, so every docs/activitymonitor/ PR gets zero labels and zero notification. (This typo also means the pre-existing label→team lookup in slack-notify-issue.yml could never match Activity Monitor issues — it was silently no-op before, but now it fails the labeling step outright.)

3. The label_count == '1' gate suppresses notifications for multi-product PRs

label-pr.yml line 60 adds all labels in a single gh pr edit call. GitHub delivers one labeled webhook per added label, but the pull_request object in each payload is serialized after the batch update, so both deliveries report labels | length == 2.

Result for a PR touching two products (e.g. docs/auditor/ + docs/pingcastle/): slack-notify-pr.yml:63 computes label_count=2 on both events, the gate at line 122 is false for both, and no "PR created" message is ever sent — nor the fork codeowners comment (line 109). Cross-product PRs are exactly the ones most in need of the notification.

Even if the ordering happened to yield one count-1 delivery, it's a race on webhook serialization, not something the workflow controls. A deterministic alternative: have label-pr.yml send the Slack notification itself right after labeling (it already knows the teams — no cross-workflow race, no counting heuristic), or keep the opened trigger and resolve teams from changed files as before.

4. slack-notify-issue.yml:53-54 has the same fragility, and doesn't fix the race it targets

Same counting approach, same failure mode when the auto-labeler applies 2+ labels in one call → LABEL_COUNT != 1 → skip → no "Issue created" message ever.

And when it does fire on a single first label, that first label is whichever one lands first — typically a category label (content:fix, bug), not the product label the auto-labeler decides later. TEAMS is then empty, so there's no team mention: the race the header comment says triggering on labeled avoids is still there, just moved.

5. PRs whose files have no mapped CODEOWNERS team now get no notification at all (regression)

Previously pull_request_target: [opened] always notified (with empty teams if nothing matched). Now the notification is downstream of a label existing, and label-pr.yml:49-51 exits cleanly when nothing matches. So these get complete Slack silence:

  • PRs touching only unowned paths — scripts/, .github/, src/, docusaurus.config.js, root config (this PR is one of them);
  • teams present in CODEOWNERS but absent from label-codeowners.json: @netwrix/platgovnetsuiteflashlight-docs, @netwrix/platgovsalesforceflashlight-docs, @netwrix/training-docs.

If silence is intended for unowned PRs, worth stating; otherwise add the missing team mappings and a fallback notification path.

6. "PR created" can fire long after the PR was created

The message text in the Build message step is keyed to action == 'labeled'. An unlabeled PR (see #5) that a human labels duplicate days later produces label_count == 1 → Slack posts "PR created: …" for a stale PR. Consider gating on the triggering label actually being in the mapping (github.event.label.name), and/or wording the message for the labeled event.

7. gh pr view --json files truncates at 100 files

label-pr.yml:44 — the GraphQL files connection is capped at 100 by gh, silently. Docs PRs here routinely exceed that (KB batches, version rollouts), so labels get resolved from a partial file list and a product label can be missed with no signal. gh api --paginate .../pulls/{n}/files avoids it. (Pre-existing behavior in the old team-resolution step, but it now determines the labels themselves.)

Minor

  • label-pr.yml:45mapfile on an empty $FILES yields a one-element array containing an empty string, which is then passed as an argv to the resolver. Harmless today, but [ -z "$FILES" ] && exit 0 is clearer.
  • label-pr.yml only ever adds labels; a synchronize that removes a product's files leaves the stale label behind (and thus keeps notifying that team).
  • resolve-labels-for-teams.mjs:33 — the !labels.includes(label) dedupe is dead code: Object.entries keys are already unique.
  • resolve-labels-for-teams.mjs uses process.cwd() for the mapping path while notify-slack.mjs resolves relative to its own file. Consistent with resolve-codeowners-teams.mjs, so fine, but it means the script only works when run from the repo root.
  • Both new label-lookup loops rely on comma as the team/label separator; safe with the current mapping, but a label containing a comma would split incorrectly.
  • sync-dev-to-main.yml — the skip path now posts to Slack on every no-op daily run, i.e. a message most days. Intentional? Teams got the same message before, so this is parity, just noting the channel noise.

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.

1 participant