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

feat!: wrap payloads to send to a "method" with "token" or "webhook" #333

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Aug 30, 2024

Summary

This PR introduces a handful of changes across this action in preparation for @v2! - #312

A few more changes might be needed, but I'm hoping this is in an alright place to test things! Most details are in the README but some testing notes are below 🙏 📚

One notable change from the proposal is the parsing of YAML values 😳 I stumbled across the js-yaml package during development and am finding that it parses super well and makes it a bit easier to author workflows - it all seems like YAML but it's parsed as a string that's converted to JSON via YAML! I think it's neat, but open to all discussion on this!

Planning to soon find ways to test this beyond local builds, but the steps for reviews can hopefully be helpful for testing things 🧪

Running experimental changes of this branch in real workflows can be done with this-
- name: Checkout the Slack GitHub Action
  uses: actions/checkout@v4
  with:
    repository: slackapi/slack-github-action
    ref: v2-development
    path: ./slack-send
    token: ${{ secrets.GITHUB_TOKEN }}

- name: Setup Node
  uses: actions/setup-node@v4
  with:
    node-version: 20
    cache: "npm"
    cache-dependency-path: ./slack-send/package-lock.json

- name: Install dependencies
  run: npm install
  working-directory: ./slack-send

- name: Post a token message into channel
  uses: ./slack-send
  with:
    method: chat.postMessage
    token: ${{ secrets.SLACK_BOT_TOKEN }}
    payload: |
      channel: ${{ secrets.SLACK_CHANNEL_ID }}
      text: "Action happens at <https://github.com/${{ github.repository }}>"

📆 If these changes are seeming alright, I'm hoping to keep this PR open for a few more weeks for possible issues to appear in testing. Please do share issues you find!

Preview

Here, the payload uses a method and token with YAML values:

- name: Post a message to a channel using a token
  uses: slackapi/slack-github-action@v2
  with:
    method: chat.postMessage
    token: ${{ secrets.SLACK_BOT_TOKEN }}
    payload: |
      text: "Actions happen at <https://github.com/${{ github.repository }}>"
      channel: ${{ secrets.SLACK_CHANNEL_ID }}

This example POSTs the payload-file-path to the webhook URL:

- name: Start a Slack workflow using a webhook URL
  uses: slackapi/slack-github-action@v2
  with:
    webhook: ${{ secrets.SLACK_WEBHOOK_URL }}
    payload-file-path: ./build-artifacts.json

Reviewers

From the kind reviewer, testing of all kinds is super appreciated! Using these changes with various token and method combinations, or testing edge cases with the payloads and inputs with webhook are all things that have changed, as well as some of the documentation that goes with this 📚

With this branch checked out, the following commands can be useful for testing on your own machine:

$ cat .github/workflows/local/README.md  # Explore details of the environment
$ vim .github/workflows/local/local.yml  # Change the action jobs to whatever
$ npm install
$ npm run local  # Run the workflows

Inspecting changes to code is also super appreciated, as always, though I'm still in the process of finishing some tests. Feel free to leave comments on anything or push directly!

Also holding off on requesting reviewers right at this moment since I'll be somewhat AFK the next weeks and slow to fast-follow fixes. Do know that I'm not meaning to ignore comments 👾

Notes

  • Some tests cover happy paths and some cover edge cases but a few still don't exist for stubbing actual API calls for the @slack/web-api in client.js and similar-
  • This change brings more files than before, but I found this made some sense during development. Open to all thoughts on this though!
  • I'm unsure if the build step is needed for distributing the action, so I makes sense to add back if it's not a certain change! Hoping to test this within the PR though 👀

⚠️ CI is failing for two reasons! One is because the pull_request_targed used in main.yml is running the workflow on main with the changes of this PR. That fails due to build setups... Another failure is due to test coverage.

Hoping that the red "x" isn't so off-putting, but I believe the pull_request workflow is the most important for our tests 🔬

Todo

  • Stub IRL callings of the @slack/web-api WebClient
  • Add the build step back if it's now breaking - it does seem needed to avoid the npm install in testing...
  • Confirm that thread_ts are being returned!
  • Revert the removal of pull_request_target in CI

Requirements

@zimeg zimeg added enhancement New feature or request semver:major labels Aug 30, 2024
@zimeg zimeg added this to the 2.0 milestone Aug 30, 2024
@zimeg zimeg self-assigned this Aug 30, 2024
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 84.97409% with 87 lines in your changes missing coverage. Please review.

Project coverage is 84.97%. Comparing base (5d1fb07) to head (0d8d929).

Files with missing lines Patch % Lines
src/client.js 69.23% 32 Missing ⚠️
src/webhook.js 74.22% 25 Missing ⚠️
src/content.js 87.01% 20 Missing ⚠️
src/config.js 94.54% 9 Missing ⚠️
src/send.js 97.22% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #333       +/-   ##
===========================================
- Coverage   97.91%   84.97%   -12.95%     
===========================================
  Files           2        6        +4     
  Lines          96      579      +483     
===========================================
+ Hits           94      492      +398     
- Misses          2       87       +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +23 to +29
- name: "build: setup the linter for formatting checks"
uses: biomejs/setup-biome@v2
with:
version: latest

- name: "unit(test): perform lints and formatting checks"
run: biome ci .
Copy link
Member Author

@zimeg zimeg Aug 30, 2024

Choose a reason for hiding this comment

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

Also a bit uncertain about this change, but found it helpful for fixing CI- It seemed like biome wasn't being downloaded in the step prior beforehand? 🤔

I was hoping npm run lint would've just worked!

https://github.com/slackapi/slack-github-action/actions/runs/10639670258/job/29498086533#step:4:13

  npm run lint
  shell: /usr/bin/bash -e {0}

> slack-github-action@2.0.0-development lint
> biome check

node:internal/modules/cjs/loader:1143
  throw err;
  ^

Error: Cannot find module '@biomejs/cli-linux-x64/biome'
Require stack:
- /home/runner/work/slack-github-action/slack-github-action/node_modules/@biomejs/biome/bin/biome
    at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
    at Function.resolve (node:internal/modules/helpers:188:19)
    at Object.<anonymous> (/home/runner/work/slack-github-action/slack-github-action/node_modules/@biomejs/biome/bin/biome:51:11)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
    at Module.load (node:internal/modules/cjs/loader:1203:32)
    at Module._load (node:internal/modules/cjs/loader:1019:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:128:12)
    at node:internal/main/run_main_module:28:49 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/slack-github-action/slack-github-action/node_modules/@biomejs/biome/bin/biome'
  ]
}

Node.js v18.20.4

@zimeg zimeg linked an issue Aug 30, 2024 that may be closed by this pull request
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Great proposal! My only concern is how we can differentiate between workflow triggers and incoming webhooks.

- name: "integration(wfb): confirm a payload was sent"
run: test -n "${{ steps.slackWorkflow.outputs.time }}"

- name: "integration(incoming): post a message via incoming webhook"
Copy link
Member

Choose a reason for hiding this comment

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

The current method requires SLACK_WEBHOOK_TYPE: INCOMING_WEBHOOK to use incoming webhooks. This is verbose, but it's clearer to check if this action step is either a workflow webhook trigger or incoming webhooks for messaging. Can we consider something similar (like webhook_type: workflow_trigger|incoming_webhooks?) to make it more descriptive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver:major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Changes for @v2
2 participants