Skip to content
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.

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

Mention author in summary message #55

Merged
merged 8 commits into from
Apr 26, 2023
Merged

Mention author in summary message #55

merged 8 commits into from
Apr 26, 2023

Conversation

namoscato
Copy link
Collaborator

@namoscato namoscato commented Apr 8, 2023

Resolves #41

Slack Deploy Pipeline Notifications example thread

Summary

  • Introduce getMessageAuthor, attempting to match GitHub user to Slack user by full name
  • Collapse package script into build
  • Rename some modules to be more descriptive
  • Introduce EnvironmentVariable enum

Release Notes

v1.3.0 attempts to match a Slack workspace user to the GitHub commit author by full name. When found, the Slack user is mentioned in the initial summary message, conventionally notifying them of all subsequent deploy messages in the thread:

Example summary message mention

This feature requires the Slack API users:read OAuth Scope which can be added to your Slack app under OAuth & Permissions. If the scope is missing or the user cannot be matched, the action will fallback to the GitHub username and avatar as before.

*/
async getRealUsers(): Promise<Member[] | null> {
try {
const {members} = await this.web.users.list()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This API method falls under Tier 2 rate limiting:

Most methods allow at least 20 requests per minute, while allowing for occasional bursts of more requests.

The Node.js Slack SDK retries such errors by default, which seems useful at the expense of increasing the GitHub workflow step duration. Probably something to keep an eye on.

const matchingSlackUsers = slackUsers.filter(
(user): user is MemberWithProfile => {
return Boolean(
user.profile?.real_name === githubUser.name &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both GitHub and Slack email APIs are separately restrictive, so I didn't want to go down that path. If we find this (naive) algorithm to be problematic, we could consider introducing some optional JSON environment variable that maps GitHub usernames to Slack member IDs (which we would have to maintain).

Choose a reason for hiding this comment

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

Yeah, I think we go with this and only start maintaining some GitHub->Slack KV structure if we have to.

Comment on lines +81 to +83
warning(
`Slack API call failed due to rate limiting. Retrying in ${numSeconds} seconds.`
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Slack Node.js SDK will also log to the console, but this will notably get surfaced in the run summary, i.e.

Screenshot 2023-04-08 at 4 44 58 PM

@namoscato namoscato marked this pull request as ready for review April 8, 2023 20:55
@namoscato
Copy link
Collaborator Author

[sc-21819]

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #21819: Mention Slack user in deploy notifications.

…ine into mention

* 'main' of github.com:namoscato/action-slack-deploy-pipeline:
  Bump json5 from 1.0.1 to 1.0.2 (#67)
  Bump @vercel/ncc from 0.34.0 to 0.36.1 (#66)
  Bump prettier from 2.7.1 to 2.8.7 (#65)
  Bump @slack/web-api from 6.7.2 to 6.8.1 (#64)
  Bump typescript from 4.8.4 to 5.0.4 (#61)
  Bump @actions/github from 5.0.3 to 5.1.1 (#60)
  Bump eslint-plugin-jest from 27.1.3 to 27.2.1 (#62)
  Bump @types/node from 16.11.56 to 16.18.23 (#63)
  Bump eslint from 8.23.0 to 8.38.0 (#59)
  Bump eslint-plugin-github from 4.3.7 to 4.7.0 (#58)
  Bump @typescript-eslint/parser from 5.40.1 to 5.57.1 (#57)
  Bump @octokit/webhooks-types from 6.3.6 to 6.11.0 (#56)
Copy link

@codybswaney codybswaney left a comment

Choose a reason for hiding this comment

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

Seems like a nice win!

Copy link

@johnkoehn johnkoehn left a comment

Choose a reason for hiding this comment

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

Looks great! Excited to get this feature shipped :D

@namoscato namoscato merged commit 3b3fa6a into main Apr 26, 2023
@namoscato namoscato deleted the mention branch April 26, 2023 19:18
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.

Mention author on failure
4 participants