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

Merged
merged 219 commits into from
Nov 14, 2024

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 in a development or testing workspace

Check out the documentation in .github/resources 🙏 ✨

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, notes on testing and changing commands can be found as resources:

🔗 https://github.com/slackapi/slack-github-action/tree/v2-development/.github/resources

Notes

  • The updated unit tests cover most happy paths and integration tests make sure messages can be sent with each technique, but I do plan to cover a few more lines in the following commits 🔬
  • This change brings more files than before, but I found this made some sense during development. Open to all thoughts on this though!
    • Also open to breaking this PR up into multiple if that's easier to review. It's nice to couple some of these changes, but I believe the testing setup is separate from actual changes to the action 😅

⚠️ CI is failing with so much change!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...

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

I'm thinking we could do some magic with what workflows exist on main while also using the pull_request_target if we're wanting to get green checks 🤔

Todo

  • Stub IRL callings of the @slack/web-api WebClient
    • Integration tests cover actual usage, but options aren't tested directly on the instantiated client 😬
  • 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!
  • Attempt to check lints in CI without additional action steps.
  • Revert the inclusion of pull_request in CI - a final step!

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
@zimeg
Copy link
Member Author

zimeg commented Nov 14, 2024

@seratch @filmaj huge thanks to y'all and the many others who've shared feedback on these changes! 🙏 ✨

I'm feeling solid about these changes and with the tests above passing I think now's an alright time to merge! Going to revert the pull_request event that was used to test CI in these changes, then follow up with a release 🚢 💨

@zimeg
Copy link
Member Author

zimeg commented Nov 14, 2024

☝️ That failure's expected since the changes to npm run build don't work with the current changes on main. Going to ignore that, but will follow up if tests continue to fail!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment