-
Notifications
You must be signed in to change notification settings - Fork 150
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
base: main
Are you sure you want to change the base?
Conversation
also fix: were the inputs not being parsed in tests idkperhaps
Codecov ReportAttention: Patch coverage is
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. |
- 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 . |
There was a problem hiding this comment.
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
There was a problem hiding this 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" |
There was a problem hiding this comment.
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?
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-
Preview
Here, the payload uses a
method
andtoken
with YAML values:This example POSTs the
payload-file-path
to thewebhook
URL:Reviewers
From the kind reviewer, testing of all kinds is super appreciated! Using these changes with various
token
andmethod
combinations, or testing edge cases with the payloads and inputs withwebhook
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:
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
@slack/web-api
inclient.js
and similar-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 👀Todo
@slack/web-api
WebClientbuild
step back if it's now breaking - it does seem needed to avoid thenpm install
in testing...thread_ts
are being returned!pull_request_target
in CIRequirements