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

Conditional notification #39

Closed
wants to merge 1 commit into from
Closed

Conditional notification #39

wants to merge 1 commit into from

Conversation

tito
Copy link
Contributor

@tito tito commented Jun 5, 2024

Here is an implementation for #38 (not yet tested)

Copy link
Owner

@shizunge shizunge left a comment

Choose a reason for hiding this comment

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

Can you add tests to tests/gantry_notify_spec.sh
We need to break test_notify_apprise into smaller tests.

There should be 4 tests:

  1. No updates, condition=all
  2. Updates, condition=all
  3. No updates, condition=change only
  4. Updates, condition=change only

@@ -98,6 +98,8 @@ You can configure the most behaviors of *Gantry* via environment variables.
| GANTRY_CLEANUP_IMAGES_OPTIONS | | [Options](https://docs.docker.com/engine/reference/commandline/service_create/#options) added to the `docker service create` command to create a global job for images removal. You can use this to add a label to the service or the containers. |
| GANTRY_NOTIFICATION_APPRISE_URL | | Enable notifications on service update with [apprise](https://github.com/caronc/apprise-api). This must point to the notification endpoint (e.g. `http://apprise:8000/notify`) |
| GANTRY_NOTIFICATION_TITLE | | Add an additional message to the notification title. |
| GANTRY_NOTIFICATION_CONDITION | all | Specifies the conditions under which notifications are sent.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you list all valid values at beginning. like line 79.

@@ -400,7 +400,17 @@ _report_services() {
local TITLE BODY
TITLE="[${STACK}] ${NUM_UPDATED} services updated ${NUM_FAILED} failed${ERROR_STRING}"
BODY=$(echo -e "${UPDATED_MSG}\n${FAILED_MSG}\n${ERROR_MSG}")
_send_notification "${TYPE}" "${TITLE}" "${BODY}"

case "${GANTRY_NOTIFICATION_CONDITION:-all}" in
Copy link
Owner

Choose a reason for hiding this comment

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

Assign global variable to a local one like line 276.
This avoids accidentally changing the global variable, or the global variable get different default values.

case "${GANTRY_NOTIFICATION_CONDITION:-all}" in
changed-only)
if [ "${NUM_UPDATED}" -gt 0 ] || [ "${NUM_TOTAL_ERRORS}" -gt 0 ]; then
_send_notification "${TYPE}" "${TITLE}" "${BODY}"
Copy link
Owner

Choose a reason for hiding this comment

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

Line 407 and 411 are same. They should be combined. like

if (some condition) {
_send_notification
}

@shizunge
Copy link
Owner

Closes due to #54

@shizunge shizunge closed this Sep 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