Skip to content

Preparation for sending non-state notifications #270

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

Jump to bottom
Open
wants to merge 6 commits into
base: icingadb-source
Choose a base branch
from
Open

Preparation for sending non-state notifications #270

wants to merge 6 commits into from

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Aug 1, 2024 •
edited
Loading

This PR introduces/extracts some of the codes of the existing components in preparation for #188. All these changes were part of that PR, but as it was slowly getting bigger and bigger, I decided to split up those commits which can be reviewed and merged independently.

  • 503b874 Contains changes for the channel plugin, that should allow us to send non-state notifications without an incident in the future and it adds also some missing doc blocks.
  • 592ac09 Just adds a simple one-line debug log in which superfluous state changes are ignored.
  • f9a9bec Is the somewhat bigger commit and also far more relevant than the others, as it extracts and/or decouples the entire rule and escalation filter evaluation from the incident package into a new common type and adds even more flexibility for Support sending non-state notifications without an incident #188. It makes it possible, for instance, to create test cases just for the filter evaluation part and also removes a number of obstacles for Support sending non-state notifications without an incident #188.
  • 39257c4 Renames the Incident#Id field to ID as we no longer need to implement an ID() receiver on Incident type, after the contracts.Incident interface has been removed by 503b874.

Blocked by

@yhabteab yhabteab requested a review from oxzi August 1, 2024 15:23
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Aug 1, 2024
@yhabteab yhabteab mentioned this pull request Aug 1, 2024
Draft
@yhabteab yhabteab force-pushed the non-state-notification-types-v1 branch 4 times, most recently from 7653b83 to 3ab100a Compare July 15, 2025 15:07
@yhabteab yhabteab removed the request for review from oxzi September 3, 2025 08:32
@yhabteab yhabteab force-pushed the non-state-notification-types-v1 branch 3 times, most recently from 30e2b00 to baedf32 Compare September 4, 2025 13:49
@yhabteab yhabteab changed the base branch from main to icingadb-source September 4, 2025 13:49
@oxzi oxzi force-pushed the icingadb-source branch 2 times, most recently from 193fb86 to de9b824 Compare September 5, 2025 15:04
@yhabteab yhabteab force-pushed the non-state-notification-types-v1 branch from baedf32 to 8dc0902 Compare September 8, 2025 08:51
@yhabteab
Copy link
Member Author

yhabteab commented Sep 8, 2025

Initially, I was thinking that this PR would become obsolete with the new event rules evaluation process, but after reviving it, it still needed for the non-state notifications PR to work properly. I've dropped all the event rules related changes from this PR, so it only contains the changes needed to evaluate escalations in a central place, plus some other (hopefully) useful refactorings and cleanups along the way (like introducing a new config.Resources type to group common resources needed for almost all operations together, without having to pass a gazillion parameters around).

PS: The tests are all failing because they depend on this Icinga Go library PR.

@yhabteab
Chanel: Allow to send notifications without an active incident
80ce28f
@yhabteab
Incident: Log ignored superfluous events
c0b67b4
@yhabteab
Offload evaluable configs from Incident to a common place
b705040
@yhabteab
Rename incident.Id -> ID & drop incident#ID()
18db26a
@yhabteab
Extract common params into a base struct & use that as an argument
9eb1097 
It's getting ridiculous that those fields have to be passed around
everywhere into functions and methods. This becomes even more ridiculous
with upcoming PR of mine, so I just bite the bullet and make a struct
for it. This struct is typically embedded into other structs that need
access to these common resources, and each function or method has just
to take the `RuntimeConfig` as a parameter, and this will be populated
from there.
@yhabteab
wip: bump IGL required version
07d937b
@yhabteab yhabteab force-pushed the non-state-notification-types-v1 branch from 8dc0902 to 07d937b Compare September 10, 2025 11:47
@yhabteab
Copy link
Member Author

I think, this PR is ready to be reviewed now. It's based on top of #324, so can't be merged before that one but can be reviewed meanwhile. Please take a look when either of you have time. Thanks! :)

oxzi
oxzi reviewed Sep 11, 2025
View reviewed changes
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

I went over the changes multiple times and in general it seems to work. However, sometimes it was a bit hard to understand the motivation for some changes.

Soft Approve.

internal/incident/incident.go
func (i *Incident) MakeNotificationRequest(ev *event.Event) *plugin.NotificationRequest {
baseUrl, err := url.Parse(daemon.Config().Icingaweb2URL)
if err != nil {
i.Logger.Panicw("Failed to parse Icinga Web 2 URL", zap.String("url", daemon.Config().Icingaweb2URL), zap.Error(err))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to panic this deep into the codebase for something which might be a misconfiguration. In the future, this even might come from the database or somewhere else. How about just returning an error?

@yhabteab
Copy link
Member Author

However, sometimes it was a bit hard to understand the motivation for some changes.

For example? You could/can also just ask me here or in person and I can telly the reasoning behind them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxzi oxzi oxzi left review comments

@julianbrost julianbrost Awaiting requested review from julianbrost

Assignees
No one assigned
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yhabteab @oxzi