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

[enhancement] Added support for Incoming Webhook #12

Merged
merged 9 commits into from
Nov 16, 2021
Merged

[enhancement] Added support for Incoming Webhook #12

merged 9 commits into from
Nov 16, 2021

Conversation

m5kr1pka
Copy link
Contributor

Summary

Closes #11 Advanced messages fails issue, because slack only accepts not flattened parameters and payload doesn't reach workflow builder afterwards.

Requirements

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2021

CLA assistant check
All committers have signed the CLA.

@li-olivierfostier
Copy link

Pleaaaase Hurry up ! :)

@akasakashota
Copy link

Did you forget to build it? I'm looking forward to this PR being merged. Thanks.

@gokaygurcan
Copy link

Any update on this?

/cc @stevengill

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.

LGTM but as I mentioned at #11 (comment), I would like to wait for @stevengill's review for this

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.

Oops, I missed that the current implementation support only Workflow Builder webhook use cases. To support both webhooks for posting a message nad webhooks for triggering a workflow, perhaps, we need to:

  • have a new option to determine the webhook type
  • dispatch the logic accordingly (=the workflow webhook pattern should be supported too)

@seratch seratch mentioned this pull request Nov 7, 2021
10 tasks
@stevengill
Copy link
Member

Agree with @seratch. I left a comment on the open issue. We don't want to remove flatten from the library but want to introduce a fork for incoming_webhook support.

@m5kr1pka
Copy link
Contributor Author

m5kr1pka commented Nov 9, 2021

cc'ing: @seratch @stevengill

  • Implemented a webhook type via env parameter: SLACK_WEBHOOK_TYPE: BUILDER or INCOMING, by default: BUILDER (existing);
  • Updated documentation with Incoming webhook;
  • added incoming webhook example to example-workflows;
  • ran build script and committed unfortunately;
  • added @vercel/ncc to devDependencies;

(phew)

P. S.
Would be great to implement tests in this action, but major release would be required

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.

A few comments. Please wait for @stevengill's review before starting to modify this PR

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@m5kr1pka
Copy link
Contributor Author

@seratch changes look good to me, thanks for the review, waiting for confirmation.

index.js Outdated Show resolved Hide resolved
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Kazuhiro Sera <seratch@gmail.com>
@stevengill stevengill merged commit a273df7 into slackapi:main Nov 16, 2021
@stevengill stevengill changed the title [fix] Removed flattening due to slack webhook only accepts not flatte… [fix] Added support for Incoming Webhook Nov 16, 2021
@stevengill stevengill changed the title [fix] Added support for Incoming Webhook [enhancement] Added support for Incoming Webhook Nov 16, 2021
@seratch seratch added this to the 1.16 milestone Nov 24, 2021
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.

Sending a message with blocks fails
10 participants