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.

Sign up for GitHub

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

Integrate Apprise notification service. #1589

Closed
wants to merge 3 commits into from
Closed

Integrate Apprise notification service. #1589

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9147f7
Integrate Apprise notification service.
Skorpios604 Jan 19, 2025
3575465
Fix validation.
Skorpios604 Jan 19, 2025
c86f23a
improve Apprise API container stability and resource management.
Skorpios604 Jan 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
APPRISE_API_URL=http://localhost:8000
Copy link
Collaborator

Choose a reason for hiding this comment

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

.env files shouldn't be committed, best to remove this commit entirely

1 change: 1 addition & 0 deletions Client/src/Components/Notifications/NotificationForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

16 changes: 15 additions & 1 deletion Server/db/models/Notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,21 @@ const NotificationSchema = mongoose.Schema(
},
type: {
type: String,
enum: ["email", "sms"],
enum: ["email", "sms", "apprise"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

},
appriseUrl: {
type: String,
validate: {
validator: function(v) {
if (this.type !== 'apprise') return true;
if (!v || v.length > 2048) return false;

// Apprise URL format: scheme://[user:pass@]host[:port]/path
const appriseUrlRegex = /^[a-zA-Z]+:\/\/([^:\s]+:[^@\s]+@)?[^\s\/:]+(:\d+)?(\/[^\s]*)?$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This regex will also allow for invalid Apprise URLs, this will match for any alphabetical string for schema, like http:// or abcd://

If we want to strictly verify Apprise URLs only, we'll have to match for all the specific protocols.

Otherwise we may as well just verify that this is a string and leave the responsibility for a valid URL with the user.

return appriseUrlRegex.test(v);
},
message: 'Invalid Apprise URL: Must be a valid service URL under 2048 characters'
}
},
address: {
type: String,
Expand Down
1 change: 1 addition & 0 deletions Server/services/NotificationService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

8 changes: 8 additions & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apprise-api:
image: caronc/apprise-api:latest
container_name: apprise-api
ports:
- "8000:8000"
environment:
- APPRISE_STATELESS_MODE=yes
restart: unless-stopped
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Yo, this Docker config needs some sauce! 🍝

My sweaty palms spotted these issues in the Docker recipe:

  1. Using latest tag is like gambling with mom's spaghetti - pin it to a specific version!
  2. No health check means we can't tell if the service is ready to serve
  3. No resource limits could lead to container eating all resources
  4. Missing newline at EOF and has trailing spaces

Here's a beefed-up config that won't make your knees weak:

 apprise-api:
-  image: caronc/apprise-api:latest
+  image: caronc/apprise-api:v1.0.0  # Replace with actual stable version
   container_name: apprise-api
   ports:
     - "8000:8000"
   environment:
     - APPRISE_STATELESS_MODE=yes
-  restart: unless-stopped 
+  restart: unless-stopped
+  healthcheck:
+    test: ["CMD", "curl", "-f", "http://localhost:8000/health"]
+    interval: 30s
+    timeout: 10s
+    retries: 3
+  deploy:
+    resources:
+      limits:
+        cpus: '0.50'
+        memory: 256M
+      reservations:
+        cpus: '0.25'
+        memory: 128M
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
apprise-api:
image: caronc/apprise-api:latest
container_name: apprise-api
ports:
- "8000:8000"
environment:
- APPRISE_STATELESS_MODE=yes
restart: unless-stopped
apprise-api:
image: caronc/apprise-api:v1.0.0 # Replace with actual stable version
container_name: apprise-api
ports:
- "8000:8000"
environment:
- APPRISE_STATELESS_MODE=yes
restart: unless-stopped
healthcheck:
test: ["CMD", "curl", "-f", "http://localhost:8000/health"]
interval: 30s
timeout: 10s
retries: 3
deploy:
resources:
limits:
cpus: '0.50'
memory: 256M
reservations:
cpus: '0.25'
memory: 128M
🧰 Tools
🪛 yamllint (1.35.1)

[error] 8-8: no new line character at the end of file

(new-line-at-end-of-file)

[error] 8-8: trailing spaces

(trailing-spaces)

Loading