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: keephq notification provider #4722

Merged
merged 7 commits into from
Apr 30, 2024
Merged

Conversation

ezhil56x
Copy link
Contributor

@ezhil56x ezhil56x commented Apr 28, 2024

  • I have read and understand the pull request rules.

Description

Added support to configure Keep as notification provider

Type of change

  • Adding new notification provider

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Hi.
Overall, the node part looks good to me.
For the frontend part I have left some thoughts inline.

Could you please attach a screenshots of the relevant events (testing/down)?

src/components/notifications/Keep.vue Outdated Show resolved Hide resolved
src/components/notifications/Keep.vue Outdated Show resolved Hide resolved
src/components/notifications/Keep.vue Outdated Show resolved Hide resolved
@@ -0,0 +1,47 @@
const NotificationProvider = require("./notification-provider");
const axios = require("axios");
const FormData = require("form-data");
Copy link
Collaborator

Choose a reason for hiding this comment

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

(in general, not only this PR)

I think we can remove this dependency in v2.0, as we have upgraded to the minimum supported node version to 18:
image

Copy link
Contributor Author

@ezhil56x ezhil56x Apr 30, 2024

Choose a reason for hiding this comment

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

Removed this as it is no longer required

src/components/notifications/Keep.vue Show resolved Hide resolved
src/components/notifications/Keep.vue Show resolved Hide resolved
src/components/notifications/Keep.vue Show resolved Hide resolved
@ezhil56x
Copy link
Contributor Author

@CommanderStorm
Thanks for taking a look into my PR. I'm currently adding support for Uptime Kuma in Keep. I will make the changes you have suggested and will attach working screenshot once my PR is merged on Keep.

@ezhil56x
Copy link
Contributor Author

@CommanderStorm
My PR has been merged in Keep. I have attached a working demo for your reference.

Click to expand
2024-04-30.14-08-08.mp4

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Thanks for the new notification provider 🎉

All changes in this PR are small and uncontroversial
⇒ merging with junior maintainer approval

@CommanderStorm CommanderStorm merged commit 988ba79 into louislam:master Apr 30, 2024
17 checks passed
@CommanderStorm CommanderStorm added this to the 2.0.0 milestone May 28, 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