fix(SA-675): Google GitHub sync fixes (#787)

* fix(SA-675): filter out suspended and archived Google users

- Add query parameter to filter suspended users at API level
- Add secondary filter in formatUserList for archived users
- Include suspended/archived fields in API response
- Add tests for suspended/archived user filtering

* fix(SA-675): exit 0 when membership changes are successfully applied

Previously, any mismatch would trigger non-zero exit even when
ADD_USERS and REMOVE_USERS were enabled and changes were made.
Now only exits non-zero when mismatch exists AND changes were
not configured to be applied (dry-run mode).

* feat(SA-675): add error handling and surface API errors

- Add OperationError interface to track individual failures
- Add OperationResult interface to collect successes and errors
- Wrap GitHub API calls in try/catch blocks
- Parse specific error codes (422, 403, 404) with helpful messages
- Surface max user count errors (422) clearly
- Collect all errors and display summary at end of run
- Exit with non-zero when errors occur

* feat(SA-675): add Slack notifications for membership changes

- Add slack.ts module with notification functionality
- Add config options: SLACK_WEBHOOK_URL, SLACK_NOTIFY_ON_ERROR,
  SLACK_NOTIFY_ON_CHANGE, SLACK_NOTIFY_ALWAYS
- Update action.yml with new Slack inputs
- Send notifications on errors (default), changes (opt-in), or always
- Format messages with users added, removed, and any errors

* chore(SA-675): update Node.js to LTS 18.20.0

Node 15.12.0 was EOL. Updated to Node 18 LTS for security and
compatibility. Major npm package updates (octokit, jest, eslint)
require careful testing due to breaking API changes and should
be addressed in a follow-up PR.

* chore(SA-675): fix lint warnings in test files

- Remove unused slack import from index.spec.ts
- Remove unused consoleSpy variable from slack.spec.ts
- Add eslint-disable comments for necessary any types in test mocks

* chore(SA-675): update deprecated GitHub Actions to v4

- actions/checkout: v3.5.2 → v4.2.2
- actions/setup-node: v3.6.0 → v4.1.0
- actions/cache: v3.3.1 → v4.2.3

Fixes CI failure due to deprecated actions/cache version being blocked
by GitHub as of Dec 2024.

Author: gjarzebak95

.github/workflows/ci.yml

@@ -14,13 +14,13 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
 
-      - uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0
+      - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0
         with:
-          node-version: 15.12.0
+          node-version: 18.20.0
 
-      - uses: actions/cache@88522ab9f39a2ea568f7027eddc7d8d8bc9d59c8 # v3.3.1
+      - uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
         with:
           path: node_modules
           key: npm-${{ hashFiles('package-lock.json') }}

.nvmrc

@@ -1 +1 @@
-16.20.0
+18.20.0

action.yml

@@ -41,6 +41,21 @@ inputs:
   github-actor:
     description: github actor to use to pull the docker image github.actor is probably fine
     required: true
+  slack-webhook-url:
+    description: 'Slack webhook URL for notifications'
+    required: false
+  slack-notify-on-error:
+    description: 'Send Slack notification when errors occur (default: true)'
+    required: false
+    default: 'true'
+  slack-notify-on-change:
+    description: 'Send Slack notification when membership changes are made'
+    required: false
+    default: 'false'
+  slack-notify-always:
+    description: 'Always send Slack notification regardless of changes'
+    required: false
+    default: 'false'
 runs:
   using: "composite"
   steps:
@@ -63,6 +78,10 @@ runs:
         -e GITHUB_INSTALLATION_ID="$GITHUB_INSTALLATION_ID" \
         -e GITHUB_PRIVATE_KEY="$GITHUB_PRIVATE_KEY" \
         -e IGNORED_USERS="$IGNORED_USERS" \
+        -e SLACK_WEBHOOK_URL="$SLACK_WEBHOOK_URL" \
+        -e SLACK_NOTIFY_ON_ERROR="$SLACK_NOTIFY_ON_ERROR" \
+        -e SLACK_NOTIFY_ON_CHANGE="$SLACK_NOTIFY_ON_CHANGE" \
+        -e SLACK_NOTIFY_ALWAYS="$SLACK_NOTIFY_ALWAYS" \
         docker.pkg.github.com/appvia/githubusermanager/githubusermanager:v1.0.5
       shell: bash
       env:
@@ -76,4 +95,8 @@ runs:
         GITHUB_INSTALLATION_ID: ${{ inputs.github-installation-id }}
         GITHUB_PRIVATE_KEY: ${{ inputs.github-private-key }}
         IGNORED_USERS: ${{ inputs.ignored-users }}
+        SLACK_WEBHOOK_URL: ${{ inputs.slack-webhook-url }}
+        SLACK_NOTIFY_ON_ERROR: ${{ inputs.slack-notify-on-error }}
+        SLACK_NOTIFY_ON_CHANGE: ${{ inputs.slack-notify-on-change }}
+        SLACK_NOTIFY_ALWAYS: ${{ inputs.slack-notify-always }}
 

index.ts

@@ -1,6 +1,7 @@
 import { getGithubUsersFromGoogle } from './src/google'
-import { getGithubUsersFromGithub, addUsersToGitHubOrg, removeUsersFromGitHubOrg } from './src/github'
+import { getGithubUsersFromGithub, addUsersToGitHubOrg, removeUsersFromGitHubOrg, OperationError } from './src/github'
 import { config } from './src/config'
+import { notifySlack } from './src/slack'
 
 export async function run(): Promise<void> {
   const googleUsers = await getGithubUsersFromGoogle()
@@ -12,17 +13,49 @@ export async function run(): Promise<void> {
   const usersNotInGithub = new Set(Array.from(googleUsers).filter((x) => !gitHubUsers.has(x)))
 
   const usersNotInGoogle = new Set(Array.from(gitHubUsers).filter((x) => !googleUsers.has(x)))
+  let unfixedMismatch = false
+  const allErrors: OperationError[] = []
+  const addedUsers: string[] = []
+  const removedUsers: string[] = []
+
   if (usersNotInGithub.size > 0) {
     console.log(`Users not in github: ${Array.from(usersNotInGithub).join(', ')}`)
-    if (config.addUsers) await addUsersToGitHubOrg(usersNotInGithub)
+    if (config.addUsers) {
+      const result = await addUsersToGitHubOrg(usersNotInGithub)
+      addedUsers.push(...result.success)
+      if (result.errors.length > 0) {
+        allErrors.push(...result.errors)
+      }
+    } else {
+      unfixedMismatch = true
+    }
   }
 
   if (usersNotInGoogle.size > 0) {
     console.log(`Users not in google: ${Array.from(usersNotInGoogle).join(', ')}`)
-    if (config.removeUsers) await removeUsersFromGitHubOrg(usersNotInGoogle)
+    if (config.removeUsers) {
+      const result = await removeUsersFromGitHubOrg(usersNotInGoogle)
+      removedUsers.push(...result.success)
+      if (result.errors.length > 0) {
+        allErrors.push(...result.errors)
+      }
+    } else {
+      unfixedMismatch = true
+    }
+  }
+
+  if (allErrors.length > 0) {
+    console.error(`\n--- ERRORS SUMMARY ---`)
+    for (const err of allErrors) {
+      console.error(`[${err.operation.toUpperCase()}] ${err.user}: ${err.message}`)
+    }
+    console.error(`Total errors: ${allErrors.length}`)
   }
 
-  const exitCode = usersNotInGoogle.size > 0 || usersNotInGithub.size > 0 ? config.exitCodeOnMissmatch : 0
+  await notifySlack(addedUsers, removedUsers, allErrors, config.githubOrg)
+
+  const hasErrors = allErrors.length > 0
+  const exitCode = unfixedMismatch || hasErrors ? config.exitCodeOnMissmatch : 0
 
   process.exit(exitCode)
 }

src/config.ts

@@ -29,6 +29,18 @@ export const config = {
   get googleEmailAddress(): string {
     return process.env.GOOGLE_EMAIL_ADDRESS ?? ''
   },
+  get slackWebhookUrl(): string | undefined {
+    return process.env.SLACK_WEBHOOK_URL
+  },
+  get slackNotifyOnError(): boolean {
+    return process.env.SLACK_NOTIFY_ON_ERROR?.toLowerCase() !== 'false'
+  },
+  get slackNotifyOnChange(): boolean {
+    return process.env.SLACK_NOTIFY_ON_CHANGE?.toLowerCase() === 'true'
+  },
+  get slackNotifyAlways(): boolean {
+    return process.env.SLACK_NOTIFY_ALWAYS?.toLowerCase() === 'true'
+  },
 }
 
 export interface googleCredentials {

src/github.ts

@@ -2,7 +2,18 @@ import { createAppAuth } from '@octokit/auth-app'
 import { Octokit } from '@octokit/rest'
 import * as mod from './github'
 import { config } from './config'
-import { GetResponseTypeFromEndpointMethod } from '@octokit/types'
+
+export interface OperationError {
+  user: string
+  operation: 'add' | 'remove'
+  message: string
+  status?: number
+}
+
+export interface OperationResult {
+  success: string[]
+  errors: OperationError[]
+}
 
 export function getAuthenticatedOctokit(): Octokit {
   return new Octokit({
@@ -57,47 +68,85 @@ export async function getUserIdFromUsername(username: string): Promise<number> {
   return user.data.id
 }
 
-export async function addUsersToGitHubOrg(users: Set<string>): Promise<void> {
+export async function addUsersToGitHubOrg(users: Set<string>): Promise<OperationResult> {
+  const result: OperationResult = { success: [], errors: [] }
   for (const user of users) {
-    await mod.addUserToGitHubOrg(user)
+    const outcome = await mod.addUserToGitHubOrg(user)
+    if (outcome === true) {
+      result.success.push(user)
+    } else if (outcome !== false && 'error' in outcome) {
+      result.errors.push(outcome.error)
+    }
   }
+  return result
 }
 
 // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
-export async function addUserToGitHubOrg(
-  user: string,
-): Promise<GetResponseTypeFromEndpointMethod<typeof octokit.orgs.createInvitation> | boolean> {
+export async function addUserToGitHubOrg(user: string): Promise<{ error: OperationError } | boolean> {
   const octokit = mod.getAuthenticatedOctokit()
   if (config.ignoredUsers.includes(user.toLowerCase())) {
     console.log(`Ignoring add for ${user}`)
     return false
   }
-  const userId = await mod.getUserIdFromUsername(user)
-  console.log(`Inviting ${user} (${userId} to ${config.githubOrg})`)
-  return await octokit.orgs.createInvitation({
-    org: config.githubOrg,
-    invitee_id: userId,
-  })
+  try {
+    const userId = await mod.getUserIdFromUsername(user)
+    console.log(`Inviting ${user} (${userId}) to ${config.githubOrg}`)
+    await octokit.orgs.createInvitation({
+      org: config.githubOrg,
+      invitee_id: userId,
+    })
+    return true
+  } catch (error) {
+    const status = error?.status || error?.response?.status
+    let message = error?.message || String(error)
+    if (status === 422) {
+      message = `Validation failed: ${message} (user may already be invited, or org is at max capacity)`
+    } else if (status === 404) {
+      message = `User not found: ${user}`
+    } else if (status === 403) {
+      message = `Permission denied or rate limited`
+    }
+    console.error(`Error adding ${user}: ${message}`)
+    return { error: { user, operation: 'add', message, status } }
+  }
 }
 
-export async function removeUsersFromGitHubOrg(users: Set<string>): Promise<void> {
+export async function removeUsersFromGitHubOrg(users: Set<string>): Promise<OperationResult> {
+  const result: OperationResult = { success: [], errors: [] }
   for (const user of users) {
-    await mod.removeUserFromGitHubOrg(user)
+    const outcome = await mod.removeUserFromGitHubOrg(user)
+    if (outcome === true) {
+      result.success.push(user)
+    } else if (outcome !== false && 'error' in outcome) {
+      result.errors.push(outcome.error)
+    }
   }
+  return result
 }
 
 // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
-export async function removeUserFromGitHubOrg(
-  user: string,
-): Promise<GetResponseTypeFromEndpointMethod<typeof octokit.orgs.removeMembershipForUser> | boolean> {
+export async function removeUserFromGitHubOrg(user: string): Promise<{ error: OperationError } | boolean> {
   const octokit = mod.getAuthenticatedOctokit()
   if (config.ignoredUsers.includes(user.toLowerCase())) {
     console.log(`Ignoring remove for ${user}`)
     return false
   }
-  console.log(`Removing user/invitation ${user} from ${config.githubOrg}`)
-  return octokit.orgs.removeMembershipForUser({
-    org: config.githubOrg,
-    username: user,
-  })
+  try {
+    console.log(`Removing user/invitation ${user} from ${config.githubOrg}`)
+    await octokit.orgs.removeMembershipForUser({
+      org: config.githubOrg,
+      username: user,
+    })
+    return true
+  } catch (error) {
+    const status = error?.status || error?.response?.status
+    let message = error?.message || String(error)
+    if (status === 404) {
+      message = `User not found or not a member: ${user}`
+    } else if (status === 403) {
+      message = `Permission denied or rate limited`
+    }
+    console.error(`Error removing ${user}: ${message}`)
+    return { error: { user, operation: 'remove', message, status } }
+  }
 }

src/google.ts

@@ -31,7 +31,8 @@ export async function getGithubUsersFromGoogle(): Promise<Set<string>> {
     customer: 'my_customer',
     maxResults: 250,
     projection: 'custom',
-    fields: 'users(customSchemas/Accounts/github(value))',
+    query: 'isSuspended=false',
+    fields: 'users(customSchemas/Accounts/github(value),suspended,archived)',
     customFieldMask: 'Accounts',
   })
 
@@ -43,6 +44,7 @@ export async function getGithubUsersFromGoogle(): Promise<Set<string>> {
 export function formatUserList(users): Set<string> {
   return new Set(
     users
+      .filter((user) => !user.suspended && !user.archived)
       .map((user) => user.customSchemas?.Accounts?.github?.map((account) => account.value?.toLowerCase()))
       .flat()
       .filter(Boolean),