Add support for alert notifications to pushmark.app#21930
Open
IGA wants to merge 1 commit intonetdata:masterfrom
Open
Add support for alert notifications to pushmark.app#21930IGA wants to merge 1 commit intonetdata:masterfrom
IGA wants to merge 1 commit intonetdata:masterfrom
Conversation
IGA
requested review from
Ancairon,
Ferroin,
kanelatechnical and
thiagoftsm
as code owners
github-actions
bot
added
area/docs
area/health
area/metadata
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 4 files
Confidence score: 3/5
- There is concrete user-impact risk in
src/health/notifications/alarm-notify.sh.in: Pushmark enablement is gated byDEFAULT_PUSHMARK_URL, which can disable role-based orDEFAULT_RECIPIENT_PUSHMARK-only notification setups before recipients are resolved. src/health/notifications/alarm-notify.sh.inalso appears to ignore resolvedto_pushmarkrecipients and always send toDEFAULT_PUSHMARK_URL, so role targeting and severity modifiers may not be honored.- The JSON/curl handling concern in
src/health/notifications/alarm-notify.sh.in(raw interpolation and bypassingdocurl) could cause malformed payloads or missing timeout/options, but confidence is low, so this is a moderate rather than blocking penalty. - Pay close attention to
src/health/notifications/alarm-notify.sh.in,src/health/notifications/pushmark/README.md- Pushmark routing/enablement behavior and endpoint documentation consistency need validation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/health/notifications/pushmark/README.md">
<violation number="1" location="src/health/notifications/pushmark/README.md:38">
P2: Pushmark docs conflict on endpoint host (`pushmark.app` vs `api.pushmark.app`), which can mislead users configuring `DEFAULT_PUSHMARK_URL`.</violation>
</file>
<file name="src/health/notifications/alarm-notify.sh.in">
<violation number="1" location="src/health/notifications/alarm-notify.sh.in:782">
P2: Pushmark enablement is tied only to DEFAULT_PUSHMARK_URL, so role-based recipients or DEFAULT_RECIPIENT_PUSHMARK-only setups are disabled before recipients are resolved.</violation>
<violation number="2" location="src/health/notifications/alarm-notify.sh.in:2565">
P2: Pushmark sender bypasses `docurl` and builds JSON via raw string interpolation, so curl options/timeouts aren’t applied and message text containing quotes/newlines can produce invalid JSON payloads.</violation>
<violation number="3" location="src/health/notifications/alarm-notify.sh.in:3855">
P2: Pushmark notifications bypass the resolved recipient list (`to_pushmark`) and always send to `DEFAULT_PUSHMARK_URL`, so role-based recipients and severity modifiers are ignored.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Agent as Netdata Agent
participant Script as alarm-notify.sh
participant Config as health_alarm_notify.conf
participant API as Pushmark API (External)
participant Client as Pushmark App (Phone/PC)
Note over Agent,Config: Initialization & Config Loading
Agent->>Script: Trigger Alarm (Status, Host, Chart, etc.)
Script->>Config: CHANGED: Load SEND_PUSHMARK & DEFAULT_PUSHMARK_URL
Config-->>Script: Return settings & recipient list
Note over Script: NEW: Pushmark logic block
alt SEND_PUSHMARK is "YES" and Recipients exist
Script->>Script: NEW: Map status to type (e.g., CRITICAL -> error)
Script->>Script: NEW: Construct JSON payload (message, type)
loop For each URL in recipients
Script->>API: NEW: POST /KEY (JSON)
alt HTTP 200 Success
API-->>Script: Success response
API->>Client: Deliver Push Notification
else HTTP Error
API-->>Script: Non-200 Response code
Script->>Script: Log error to stderr
end
end
end
Script-->>Agent: Return exit code (0 if any sent, 1 if all failed)
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| | Option | Description | Default | Required | | ||
| |:-----------------------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------|:---------:| | ||
| | SEND_PUSHMARK | Set `SEND_PUSHMARK` to YES | YES | yes | | ||
| | [DEFAULT_PUSHMARK_URL](#option-default-recipient-pushmark) | URL formed by the server-topic combination you want the alert notifications to be sent to. Unless hosting your own server, the server should always be set to https://pushmark.app. | | yes | |
Contributor
There was a problem hiding this comment.
P2: Pushmark docs conflict on endpoint host (pushmark.app vs api.pushmark.app), which can mislead users configuring DEFAULT_PUSHMARK_URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/health/notifications/pushmark/README.md, line 38:
<comment>Pushmark docs conflict on endpoint host (`pushmark.app` vs `api.pushmark.app`), which can mislead users configuring `DEFAULT_PUSHMARK_URL`.</comment>
<file context>
@@ -0,0 +1,109 @@
+| Option | Description | Default | Required |
+|:-----------------------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------|:---------:|
+| SEND_PUSHMARK | Set `SEND_PUSHMARK` to YES | YES | yes |
+| [DEFAULT_PUSHMARK_URL](#option-default-recipient-pushmark) | URL formed by the server-topic combination you want the alert notifications to be sent to. Unless hosting your own server, the server should always be set to https://pushmark.app. | | yes |
+
+<a id="option-default-recipient-pushmark"></a>
</file context>
| [ -z "${DEFAULT_RECIPIENT_NTFY}" ] && SEND_NTFY="NO" | ||
|
|
||
| # check pushmark | ||
| [ -z "${DEFAULT_PUSHMARK_URL}" ] && SEND_PUSHMARK="NO" |
Contributor
There was a problem hiding this comment.
P2: Pushmark enablement is tied only to DEFAULT_PUSHMARK_URL, so role-based recipients or DEFAULT_RECIPIENT_PUSHMARK-only setups are disabled before recipients are resolved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/health/notifications/alarm-notify.sh.in, line 782:
<comment>Pushmark enablement is tied only to DEFAULT_PUSHMARK_URL, so role-based recipients or DEFAULT_RECIPIENT_PUSHMARK-only setups are disabled before recipients are resolved.</comment>
<file context>
@@ -769,6 +778,9 @@ fi
[ -z "${DEFAULT_RECIPIENT_NTFY}" ] && SEND_NTFY="NO"
+# check pushmark
+[ -z "${DEFAULT_PUSHMARK_URL}" ] && SEND_PUSHMARK="NO"
+
# check msteams
</file context>
|
|
||
| # ----------------------------------------------------------------------------- | ||
| # send messages to pushmark | ||
| send_pushmark "${DEFAULT_PUSHMARK_URL}" |
Contributor
There was a problem hiding this comment.
P2: Pushmark notifications bypass the resolved recipient list (to_pushmark) and always send to DEFAULT_PUSHMARK_URL, so role-based recipients and severity modifiers are ignored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/health/notifications/alarm-notify.sh.in, line 3855:
<comment>Pushmark notifications bypass the resolved recipient list (`to_pushmark`) and always send to `DEFAULT_PUSHMARK_URL`, so role-based recipients and severity modifiers are ignored.</comment>
<file context>
@@ -3799,6 +3850,11 @@ SENT_GOTIFY=$?
+# -----------------------------------------------------------------------------
+# send messages to pushmark
+send_pushmark "${DEFAULT_PUSHMARK_URL}"
+SENT_PUSHMARK=$?
+
</file context>
Suggested change
| send_pushmark "${DEFAULT_PUSHMARK_URL}" | |
| send_pushmark "${to_pushmark}" |
| esac | ||
|
|
||
| for recipient in ${recipients}; do | ||
| httpcode=$(curl -s -o /dev/null -w "%{http_code}" -X POST ${recipient} \ |
Contributor
There was a problem hiding this comment.
P2: Pushmark sender bypasses docurl and builds JSON via raw string interpolation, so curl options/timeouts aren’t applied and message text containing quotes/newlines can produce invalid JSON payloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/health/notifications/alarm-notify.sh.in, line 2565:
<comment>Pushmark sender bypasses `docurl` and builds JSON via raw string interpolation, so curl options/timeouts aren’t applied and message text containing quotes/newlines can produce invalid JSON payloads.</comment>
<file context>
@@ -2528,6 +2543,42 @@ send_ntfy() {
+ esac
+
+ for recipient in ${recipients}; do
+ httpcode=$(curl -s -o /dev/null -w "%{http_code}" -X POST ${recipient} \
+ -H "Content-Type: application/json" \
+ -d "{\"message\": \"$msg\", \"type\": \"$type\"}")
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds the option to send push notifications to phones and/or PCs via PUT/POST requests, using https//pushmark.app.
Test Plan
Using the branch of this PR:
SEND_PUSHMARK="YES"inhealth_alarm_notify.conf.DEFAULT_PUSHMARK_URLinhealth_alarm_notify.confwith one or more recipients, such as:Additional Information
For users: How does this change affect me?
It adds support for sending `pushmark.app` notifications, in a pub/sub way.Summary by cubic
Add Pushmark notification support to send Netdata alerts to phones/PCs via simple HTTP POSTs. Enable with one config flag and point to one or more Pushmark URLs to start receiving alerts.
alarm-notify.sh(POST JSON withmessageandtype).health_alarm_notify.conf:SEND_PUSHMARK,DEFAULT_PUSHMARK_URL(supports multiple recipients).role_recipients_pushmark[...].DEFAULT_RECIPIENT_PUSHMARK; auto-disables when no URL is set.pushmark/README.md) and metadata (pushmark/metadata.yaml).Written for commit 65aeacd. Summary will update on new commits.