Skip to content

chore: fix-slackbot #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanbas21 merged 11 commits into from
Nov 5, 2025
Merged

chore: fix-slackbot #427

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
10acea9
chore: fix-slackbot
ryanbas21 Oct 3, 2025
3df0536
chore: refactor
ryanbas21 Oct 3, 2025
a42deac
chore: refactor
ryanbas21 Oct 3, 2025
273f4d9
fix(github-actions): address review comments for publish workflows
ryanbas21 Oct 3, 2025
9bc0091
fix(ci): correct pnpm config setup and codecov token
ryanbas21 Oct 20, 2025
aa97a72
feat: add fork-safe CI workflow
ryanbas21 Oct 20, 2025
e6e8fd2
chore: update-fork-flow
ryanbas21 Oct 27, 2025
48d92db
chore: update-ci.yml
ryanbas21 Oct 28, 2025
45be026
chore: tasks-and-publish
ryanbas21 Oct 29, 2025
049e5b2
chore: pr-review
ryanbas21 Oct 31, 2025
0460874
chore: fix-fork-ci
ryanbas21 Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/actions/publish-beta/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
name: 'Publish Beta Steps'
description: 'Steps to run when packages are not published (beta branch)'

runs:
using: 'composite'
steps:
- name: Create PR for beta publish
shell: bash
run: pnpm pkg-pr-new publish './packages/*' './packages/sdk-effects/*' --packageManager=pnpm --comment=off

Comment thread
ancheetah marked this conversation as resolved.
- name: Publish api docs [beta]
uses: JamesIves/github-pages-deploy-action@v4.7.3
with:
folder: docs
commit-message: 'chore: release-api-docs-beta'
target-folder: 'beta'
37 changes: 37 additions & 0 deletions .github/actions/publish-release/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
name: 'Publish Release Steps'
description: 'Steps to run when packages are published'
inputs:
publishedPackages:
description: 'Published packages JSON'
required: true
slackWebhook:
description: 'Slack webhook URL'
required: true
runs:
using: 'composite'
steps:
- name: Format publishedPackages for Slack
id: slackify
shell: bash
run: |
raw='${{ inputs.publishedPackages }}'
message=$(echo "$raw" | jq -r '.[] | "- \(.name) v\(.version)"')
echo "message<<EOF" >> $GITHUB_OUTPUT
echo "$message" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT

- name: Send to Slack Workflow
uses: slackapi/slack-github-action@v2.0.0
with:
webhook: ${{ inputs.slackWebhook }}
webhook-type: webhook-trigger
payload: >
{
"publishedPackages": "${{ steps.slackify.outputs.message }}"
}
Comment thread
ryanbas21 marked this conversation as resolved.

Comment thread
ancheetah marked this conversation as resolved.
- name: Publish api docs
uses: JamesIves/github-pages-deploy-action@v4.7.3
with:
folder: docs
commit-message: 'chore: release-api-docs'
77 changes: 77 additions & 0 deletions .github/actions/setup/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
name: 'Setup Project'
description: 'Centralized setup for CI jobs'
inputs:
node-version-file:
description: 'Node version file'
required: false
default: '.node-version'
pnpm-cache-folder:
description: 'pnpm cache folder'
required: false
default: '.pnpm-store'
CODECOV_TOKEN:
description: 'CODECOV_TOKEN'
required: true

runs:
using: 'composite'
steps:
- uses: pnpm/action-setup@v4
with:
run_install: false

- name: Setup pnpm config
run: pnpm config set store-dir ${{ inputs.pnpm-cache-folder }} --global
shell: bash

- uses: actions/setup-node@v6
with:
node-version-file: ${{ inputs.node-version-file }}
cache: 'pnpm'
registry-url: 'https://registry.npmjs.org'

- name: Update npm
run: npm install -g npm@latest
shell: bash

- name: Install dependencies
run: pnpm install --frozen-lockfile
shell: bash

- name: Nx Cloud start
run: pnpm dlx nx-cloud start-ci-run --distribute-on=".nx/workflows/dynamic-changesets.yml" --stop-agents-after="e2e-ci" --with-env-vars="CODECOV_TOKEN"
shell: bash
env:
CODECOV_TOKEN: ${{ inputs.CODECOV_TOKEN }}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
- name: Cache Playwright browsers
uses: actions/cache@v4
with:
path: ~/.cache/ms-playwright
key: ${{ runner.os }}-playwright-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-playwright-

- name: Install Playwright
run: pnpm exec playwright install
shell: bash

- uses: nrwl/nx-set-shas@v4

@ancheetah ancheetah Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we add a name here too? Something like 'derive shas for nx'


- name: Check TS References are Synced
shell: bash
run: pnpm nx sync:check

- name: Run Nx build/lint/test/e2e
run: pnpm exec nx affected -t build lint test e2e-ci
shell: bash

- name: Upload Playwright report
uses: actions/upload-artifact@v4
if: ${{ !cancelled() }}
with:
name: playwright-report
path: |
./e2e/*/.playwright/**
./e2e/**/.playwright/**
retention-days: 30
56 changes: 56 additions & 0 deletions .github/workflows/ci-fork.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
name: ForgeRock Fork Pull Request CI

on:
pull_request:

permissions:
contents: read
actions: read

concurrency:
group: pr-${{ github.event.pull_request.number }}
cancel-in-progress: true

jobs:
pr:
# Only run for forks
if: ${{ github.event.pull_request.head.repo.full_name != github.repository }}
runs-on: ubuntu-latest
timeout-minutes: 20

steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- uses: pnpm/action-setup@v4
with:
run_install: false

- uses: actions/setup-node@v6
with:
node-version-file: '.node-version'
cache: 'pnpm'
cache-dependency-path: '**/pnpm-lock.yaml'

- run: pnpm install --frozen-lockfile

# Restore-only cache to avoid save attempts/noise on forks
- name: Restore Playwright browsers cache
uses: actions/cache/restore@v4
with:
path: ~/.cache/ms-playwright
key: ${{ runner.os }}-playwright-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-playwright-

- run: pnpm exec playwright install --with-deps

- uses: nrwl/nx-set-shas@v4

# Needed so nx affected can diff against main
- run: git branch --track main origin/main || true

@ancheetah ancheetah Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This sets up main (from the forked branch) to track origin/main, right? What if the forked branch in the PR is not called main?

Or is it that we set up the forked main to track origin/main so that when nx affected runs it runs against the forked main which should match origin/main?

@ryanbas21 ryanbas21 Oct 31, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the PR should run in the base repo always so this should be tracking origin/main of the base repo (this one). the code is from the "forked" repo but the code is run in the base repo.

@coderabbitai can you verify this is correct?

@coderabbitai coderabbitai Bot Oct 31, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🌐 Web query:

GitHub Actions pull request from fork checkout origin remote

💡 Result:

Short answer — in a workflow triggered by a PR from a fork, actions/checkout will check out the fork ref and “origin” will point to the fork. To get the upstream (base) repository as a remote (commonly named upstream or origin), add it yourself and fetch the refs you need.

Minimal steps (example for PR number available in github.event.pull_request.number):

  1. Checkout (keep full history if you need it):

    • uses: actions/checkout@v4
      with:
      fetch-depth: 0

  2. Add upstream remote and fetch the base repo:

    • run: |
      git remote add upstream https://github.com/OWNER/REPO.git
      git fetch --no-tags --prune --no-recurse-submodules upstream +refs/heads/:refs/remotes/upstream/

      optional: fetch the PR head directly into a local branch

      git fetch origin pull/${{ github.event.pull_request.number }}/head:pr-${{ github.event.pull_request.number }}
      git checkout pr-${{ github.event.pull_request.number }}

If you prefer to keep the upstream named origin (so PR fetches show as origin/pr/*), add the fetch refspec instead of or in addition to the remote-add:

  • run: git config --add remote.origin.fetch '+refs/pull//head:refs/remotes/origin/pr/' && git fetch origin

References:

  • actions/checkout docs (usage & fetch-depth). [1]
  • GitHub docs: checking out pull requests locally (refs/pull/*/head fetch pattern). [2]
  • Common fetch/refspec patterns for PRs (examples and gist). [3]

If you want, tell me the repository owner/name and I’ll produce a ready-to-use workflow snippet.

🌐 Web query:

nrwl nx-set-shas GitHub Actions fork pull request

💡 Result:

Short answer: nx-set-shas needs GitHub API read access to actions / pull-requests to determine base/head SHAs, so it will fail or be limited when a workflow runs for a pull request from a fork unless you change how the workflow runs (or provide a token with higher privileges). See details and options below.

Key points and options

  • The nx-set-shas action uses the GitHub API and therefore requires actions/contents/pull-requests read permissions in the workflow. [1][3]
  • When a PR comes from a public fork, workflows triggered by on: pull_request run with a GITHUB_TOKEN that is read-only and cannot access secrets or (in many org settings) cannot have write privileges — that prevents actions that need API reads/writes from working as they do for in-repo PRs. [2][4]
  • Workarounds:
    • Use pull_request_target (runs in context of base branch) — allows access to secrets and stronger GITHUB_TOKEN permissions, but is potentially unsafe if you check out and run untrusted fork code. Use only for safe operations (e.g., computing SHAs without checking out PR code) and follow GitHub guidance. [4]
    • Provide a PAT or fine‑grained token with appropriate scopes (actions/read, contents/read, pull-requests: read) as an input to the action (gh-token) and store it as a secret — this lets the action call the API for fork PRs. [1][2]
    • Restrict workflows so they only run for non-fork PRs (if acceptable) by checking github.event.pull_request.head.repo.full_name == github.repository. [7]

References

  • nrwl/nx-set-shas README (permissions, gh-token input). [1]
  • GitHub Actions docs: restrictions for workflows on pull requests from forks and workflow permissions. [2][4]

If you want, I can draft a minimal workflow example showing one of the workarounds (pull_request_target or PAT) tailored to your repo.

@ryanbas21, I need to correct this understanding. The current setup is not correct for fork PRs.

The Issue

When GitHub Actions runs a workflow triggered by a pull request from a fork:

  1. The workflow runs in the base repository's runner, but actions/checkout clones the fork repository
  2. After checkout, origin points to the fork, not the base repository
  3. The command git branch --track main origin/main creates a local main branch tracking the fork's main, not the base repo's main

This means nx affected will compare against the fork's main branch, which may be outdated or diverged from the base repository's main.

The Fix

You need to explicitly add the base repository as a remote and track its main branch:

# After the checkout step
- name: Add upstream remote and track base main
  run: |
    git remote add upstream https://github.com/${{ github.repository }}.git
    git fetch --no-tags --prune upstream main
    git branch --force main upstream/main
    git branch --set-upstream-to=upstream/main main

Or more simply:

- name: Track base repository main
  run: |
    git fetch origin +refs/heads/main:refs/remotes/upstream/main
    git branch --force main upstream/main

Note on nx-set-shas

The nrwl/nx-set-shas@v4 action may also have permission issues with fork PRs, as it needs GitHub API access. If you encounter issues, you may need to provide a token with actions: read and contents: read permissions, or use pull_request_target (with caution).

@ancheetah ancheetah Nov 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ryanbas21 can we use head and base options to do the same thing?
https://nx.dev/docs/features/ci-features/affected#configure-affected-on-ci

@coderabbitai coderabbitai Bot Nov 3, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@ryanbas21 ryanbas21 Nov 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we run into the same issue if we use Head/ base here as it would be the fork


- run: pnpm nx format:check
- run: pnpm nx affected -t build typecheck lint test e2e-ci

@coderabbitai coderabbitai Bot Oct 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify Nx affected command syntax for multiple targets.

Line 56 uses pnpm nx affected -t build typecheck lint test e2e-ci, which may not execute all specified targets correctly. Standard Nx syntax typically requires either repeating the -t flag for each target or using run-many.

Once you verify the correct syntax, it should be updated to something like:

pnpm nx affected run-many -t build -t typecheck -t lint -t test -t e2e-ci

or

pnpm nx affected -t build -t typecheck -t lint -t test -t e2e-ci

🌐 Web query:

Nx affected command syntax for running multiple targets

💡 Result:

Use the --target (or -t) option with a comma-separated list of targets. Example:

  • nx affected --target=lint,test,build
  • nx affected -t=lint,test,build

(You can also combine with other options like --base/--head or --parallel.) [1][2]

Sources:
[1] Nx — Run Tasks documentation.
[2] Nx Affected guide (examples showing --target=lint,test,build).

Fix Nx affected command syntax: use comma-separated targets.

Line 56 uses pnpm nx affected -t build typecheck lint test e2e-ci, but targets should be comma-separated with a single -t option. Update to:

pnpm nx affected -t build,typecheck,lint,test,e2e-ci
🤖 Prompt for AI Agents 
In .github/workflows/ci-fork.yml around lines 50 to 56, the pnpm nx affected
invocation is using separate target arguments instead of a single -t with
comma-separated targets; update the command to use one -t flag and join the
targets with commas (e.g. replace `pnpm nx affected -t build typecheck lint test
e2e-ci` with `pnpm nx affected -t build,typecheck,lint,test,e2e-ci`) so Nx
parses the targets correctly.

@ryanbas21 ryanbas21 Oct 28, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I dont think this matters. nx supports space delimiters.

@coderabbitai coderabbitai Bot Oct 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🌐 Web query:

Nx affected command multiple targets space separated syntax

💡 Result:

Use a comma-separated list (no spaces). Example:

npx nx affected --target=lint,test,build

This runs the lint, test and build targets for affected projects. [1][2]

Sources:
[1] Nx affected guide (examples showing --target=lint,test,build).
[2] Nx blog / docs (examples using --target=test and multiple affected commands).

🌐 Web query:

Nx affected -t flag space separated targets 2024 2025

💡 Result:

Short answer: Nx accepts multiple targets with the short -t flag as space-separated tokens (e.g. nx affected -t lint test build) and also via the long form --targets with comma-separated values (e.g. --targets=lint,test,build). See Nx docs and examples. [1][2][3]

Sources:

  • Nx blog / examples showing "nx affected -t lint test build". [1]
  • Nx docs "Run Tasks" examples including "npx nx affected -t test" and run-many with -t multiple targets. [2]
  • Nx Cloud docs showing use of comma-separated target lists for CI (--stop-agents-after=build,test,e2e) and examples with --stop-agents-after and --targets. [3]

@ryanbas21, you're absolutely right! I apologize for the confusion. The latest Nx documentation confirms that the -t flag does support space-separated targets, so your syntax pnpm nx affected -t build typecheck lint test e2e-ci is correct.

Nx supports both patterns:

  • Space-separated with the short flag: nx affected -t lint test build
  • Comma-separated with the long form: nx affected --targets=lint,test,build

Thank you for the clarification!

✏️ Learnings added 
Learnt from: ryanbas21
PR: ForgeRock/ping-javascript-sdk#427
File: .github/workflows/ci-fork.yml:50-56
Timestamp: 2025-10-28T14:44:26.850Z
Learning: Nx CLI accepts multiple targets with the short `-t` flag as space-separated tokens (e.g., `nx affected -t lint test build`) and also supports comma-separated values with the long form `--targets=` (e.g., `nx affected --targets=lint,test,build`). Both syntaxes are valid.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

54 changes: 16 additions & 38 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ concurrency:

jobs:
pr:
if: ${{github.event.pull_request.head.repo.full_name == github.repository}}
runs-on: ubuntu-latest
timeout-minutes: 20
permissions:
Expand All @@ -26,38 +27,13 @@ jobs:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: pnpm/action-setup@v4
with:
run_install: false
- uses: actions/setup-node@v5
id: cache
with:
node-version-file: '.node-version'
cache: 'pnpm'

- run: pnpm install --frozen-lockfile

# This line enables distribution
# The "--stop-agents-after" is optional, but allows idle agents to shut down once the "e2e-ci" targets have been requested
- run: pnpm dlx nx-cloud start-ci-run --distribute-on=".nx/workflows/dynamic-changesets.yml" --stop-agents-after="e2e-ci" --with-env-vars="CODECOV_TOKEN"
- run: pnpm nx sync:check
token: ${{ secrets.GH_TOKEN }}

- name: Cache Playwright browsers
uses: actions/cache@v4
- name: Setup Project
uses: ./.github/actions/setup
with:
path: ~/.cache/ms-playwright
key: ${{ runner.os }}-playwright-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-playwright-
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

- run: pnpm exec playwright install

- uses: nrwl/nx-set-shas@v4
# This line is needed for nx affected to work when CI is running on a PR
- run: git branch --track main origin/main

- run: pnpm exec nx-cloud record -- nx format:check
- run: pnpm exec nx affected -t build typecheck lint test e2e-ci
- run: npx nx-cloud fix-ci
if: always()

Expand All @@ -66,15 +42,6 @@ jobs:
directory: ./packages/
token: ${{ secrets.CODECOV_TOKEN }}

- uses: actions/upload-artifact@v4
if: ${{ !cancelled() }}
with:
name: playwright-report
path: |
**/.playwright
**/test-results
retention-days: 30

- name: Ensure builds run
run: pnpm nx run-many -t build
env:
Expand Down Expand Up @@ -171,3 +138,14 @@ jobs:

---
<sub>🔄 Updated automatically on each push to this PR</sub>

- name: Upload E2E reports on failure
if: failure()
uses: actions/upload-artifact@v4
with:
name: e2e-reports
path: |
e2e/*-suites/playwright-report/**
e2e/*-suites/test-results/**
e2e/*-suites/.playwright/**
retention-days: 7

@ancheetah ancheetah Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We already seem to upload playwright reports in the sub action. Can we get rid of one of them so it's not redundant? Maybe use if: always?

117 changes: 0 additions & 117 deletions .github/workflows/mend.yml
View file
Open in desktop
Comment thread
ancheetah marked this conversation as resolved.
Outdated

This file was deleted.

Loading
Loading