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

Add example usage of outgoing webhooks #171

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Add example usage of outgoing webhooks #171

merged 3 commits into from
Dec 18, 2023

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Nov 8, 2023

Summary

Screenshot from 2023-11-08 15-08-42

Ticket Link

For mattermost/mattermost#23805

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (308bdd6) 2.93% compared to head (3ce8588) 3.10%.

Files Patch % Lines
server/http_hooks.go 16.00% 21 Missing ⚠️
server/utils.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master    #171      +/-   ##
=========================================
+ Coverage    2.93%   3.10%   +0.16%     
=========================================
  Files          14      15       +1     
  Lines        1838    1869      +31     
=========================================
+ Hits           54      58       +4     
- Misses       1782    1809      +27     
  Partials        2       2              

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

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Nov 9, 2023
@DHaussermann
Copy link

/update-branch

@DHaussermann
Copy link

@hanzei I have a bit of a silly problem. I can't get the build for this.
Locally when I build I get these errors. I have tried using various versions of node and it did not solve it.
image

Perhaps more important than whatever is happening on my local... I see the pipeline can build it just fine but I don't know how to go get the artifact. Has this UI changed?
image
Where do I go get this? I think there use to be a grren button in the UI that I no longer see. But that may have never been the case for this repo.

Please let me know if you can provide some guidance.

@hanzei
Copy link
Contributor Author

hanzei commented Dec 4, 2023

Can you please run nvm use to select the right version?

@DHaussermann
Copy link

DHaussermann commented Dec 7, 2023

@hanzei I may ned help on this again.
I'm afraid this isn't related to your PR but I can't make a working outgoing webhook to test with. I set the callback to some URL where the is no app waiting for the request but that should not matter for what I'm doing.

I see an error when it tries to fire the hook.

{
  "caller": "app/webhook.go:120",
  "error": "doOutgoingWebhookRequest: Failed to unmarshal., invalid character 'T' looking for beginning of value",
  "ip_addr": "74.15.38.100",
  "level": "error",
  "method": "POST",
  "msg": "Event POST failed.",
  "path": "/api/v4/posts",
  "request_id": "kneiybqcet868b34tircbf7tsr",
  "timestamp": "2023-12-07 22:19:10.556 Z",
  "user_id": "sn41xt7ktjyapy36xn454f144c"
}

Please advise what I may be missing here. It's not clear to me what value stats with T that I need to adjust.

@DHaussermann
Copy link

@hanzei I tried this again but still no luck.

I made a valid outgoing webhook to a server running locally and the demo plugin did not do anything. Which is probably expected.
image

I also read the documentation updates and you provided and pointed the hook to http://localhost:8065/plugins/com.mattermost.demo-plugin/webhook/outgoing but also saw no reposnse.

Let me know if you have any suggestions.

@hanzei
Copy link
Contributor Author

hanzei commented Dec 14, 2023

@DHaussermann Can you please share a screenshot of settings that your Outgoing Webhook uses?

@DHaussermann
Copy link

DHaussermann commented Dec 14, 2023

@hanzei When I set this up again on a new /cloud server it was working. Possibly there was a typo or some other config error. It's just strange because I also tried locally with a localhost:8065 calback URL
Probably of little interest to you know but here is the config...
image

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Demo plugin now provided a mechanism for testing outgoing webhook that posts a response as expected
  • Setup is documented.
  • Delay setting in plugin config is respected.
  • I ended up not needing the logging but I did try with an incorrect callback URL and confirmed and error clearly shows a failure for an outgoing webhook and provides details.

LGTM!

Huge thanks @hanzei this is a great enhancement that will help facilitate future testing! 🎉

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 14, 2023
@hanzei hanzei merged commit d6a03f3 into master Dec 18, 2023
6 checks passed
@hanzei hanzei deleted the outgoing_webhooks branch December 18, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants