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 pino-discord-webhook transport information #1922

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Add pino-discord-webhook transport information #1922

merged 1 commit into from
Mar 20, 2024

Conversation

fabulousgk
Copy link
Contributor

This PR adds a discord webhook transport to the list of known transports.

@mcollina
Copy link
Member

Can you please add a few tests?
How could someone get the webhook url? Are there security tokens involved?

@fabulousgk
Copy link
Contributor Author

I added information about how to create a webhook and get the URL.

I have tried adding testing, but I have no experience with test suites in JS. I looked at the Slack one, but my setup is a bit different.

The main issue is I am not sure how to test the actual sending as that is wrapped in the callback function.

So I can test that the transport is created, but nothing beyond that. If someone gives me hints I will add it.

I will alos ask elsewere.

I noticed that there are transports without tests, is this required for publising?

@mcollina
Copy link
Member

I noticed that there are transports without tests, is this required for publising?

I double checked and only the logtail one does not have tests. This is unfortunate and it slipped.

Overally, yes, I consider tests to be a requirement for inclusion in this list.


To test this I would spin up an http server and pass its URL as the webhook. Then, verify this endpoint is called with the expected output.

@fabulousgk
Copy link
Contributor Author

Well, I added a test. The idea of a webserver seemed not impossible since what I am doing is simply calling a class in the discord.js library, which handles the rest of the interaction. So I am testing that that call occurs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 31d4fac into pinojs:master Mar 20, 2024
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.

2 participants