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: do not store notification events to job history. Keep track of any filter errors in the notification table itself. #570

Merged
merged 9 commits into from
Sep 27, 2023

Conversation

adityathebe
Copy link
Member

@adityathebe adityathebe commented Sep 21, 2023

Resolves: #542
Merge after: flanksource/duty#287

@adityathebe adityathebe force-pushed the chore/notification-job-history branch 2 times, most recently from 5ce0666 to 5629055 Compare September 26, 2023 08:54
@adityathebe adityathebe changed the title chore: always save notification to job history feat: Keep a history of all notifications that are sent & Maintain the error status of a notification. Sep 26, 2023
@adityathebe adityathebe changed the title feat: Keep a history of all notifications that are sent & Maintain the error status of a notification. feat: do not store notification events to job history. Keep track of a ny filter errors in the notification table itself. Sep 26, 2023
@adityathebe
Copy link
Member Author

@moshloop On a second look, I think what we currently have in events/notifications.go file should stay there and shouldn't be move the notification package.

I see the notification package as our mail package which is just the client to send notification. It can be called from any other places to send a notification. So, I think it shouldn't hold in any logic for event queue handling like

  • creating a notification.send event from check.failed event, or
  • deciding the title and body for incident related events.

We could move to a new notification/events package instead but I still think what we currently have is fine.

@adityathebe adityathebe force-pushed the chore/notification-job-history branch from d7d998a to 6a40995 Compare September 26, 2023 11:26
@moshloop
Copy link
Member

moshloop commented Sep 26, 2023

@adityathebe - notifications.go is still too large

  • These all should be moved to notification package and the prefix removed: NotificationEventPayload, NotificationTemplate, labelsTemplate, defaultTitleAndBody, sendNotification
  • addNotificationEvent should be trimmed down to only forwarding the relevant details to notification package - e.g. GetNotifications should only be called from within the notification package,
  • getEnvForEvent should remain in events.go

@adityathebe adityathebe force-pushed the chore/notification-job-history branch 2 times, most recently from 1708729 to 15ed4b5 Compare September 27, 2023 11:24
@adityathebe adityathebe force-pushed the chore/notification-job-history branch from 15ed4b5 to ed98ed1 Compare September 27, 2023 11:58
@adityathebe adityathebe changed the title feat: do not store notification events to job history. Keep track of a ny filter errors in the notification table itself. feat: do not store notification events to job history. Keep track of any filter errors in the notification table itself. Sep 27, 2023
@adityathebe adityathebe force-pushed the chore/notification-job-history branch from ed98ed1 to a4edadd Compare September 27, 2023 12:11
@adityathebe adityathebe force-pushed the chore/notification-job-history branch from a4edadd to 48848f4 Compare September 27, 2023 12:27
@adityathebe
Copy link
Member Author

@moshloop I've made the changes.

@moshloop moshloop merged commit 72549a3 into main Sep 27, 2023
5 checks passed
@moshloop moshloop deleted the chore/notification-job-history branch September 27, 2023 13:44
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.

When notification filter compile errors are resolved, the status still shows as failed
2 participants