-
Notifications
You must be signed in to change notification settings - Fork 0
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
Write schedule/group_id
for contact notified histories
#64
base: main
Are you sure you want to change the base?
Conversation
Another thing I just noticed which would be nice: If a schedule with no entry at the current time or an empty group is notified (i.e. no notification actually happened because it resolved to no contact), writing a row with |
c55d0aa
to
272efd5
Compare
@@ -33,6 +33,24 @@ func (r Key) MarshalText() (text []byte, err error) { | |||
} | |||
} | |||
|
|||
// CopyNonNil copies non-nil fields from the provided recipient.Key to the current one and | |||
// returns the modified recipient key. | |||
func (r Key) CopyNonNil(recipientKey Key) Key { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function and how you use recipient.Key
in incident.HistoryRow
contradicts the current assumption in the code that there's a 1:1 relation between recipient.Recipient
and recipient.Key
, as it can be seen in multiple places:
icinga-notifications/internal/recipient/recipient.go
Lines 16 to 21 in 002d14d
type Key struct { | |
// Only one of the members is allowed to be a non-zero value. | |
ContactID types.Int `db:"contact_id"` | |
GroupID types.Int `db:"contactgroup_id"` | |
ScheduleID types.Int `db:"schedule_id"` | |
} |
icinga-notifications/internal/recipient/recipient.go
Lines 23 to 34 in 002d14d
// MarshalText implements the encoding.TextMarshaler interface to allow JSON marshaling of map[Key]T. | |
func (r Key) MarshalText() (text []byte, err error) { | |
if r.ContactID.Valid { | |
return []byte(fmt.Sprintf("contact_id=%d", r.ContactID.Int64)), nil | |
} else if r.GroupID.Valid { | |
return []byte(fmt.Sprintf("group_id=%d", r.GroupID.Int64)), nil | |
} else if r.ScheduleID.Valid { | |
return []byte(fmt.Sprintf("schedule_id=%d", r.ScheduleID.Int64)), nil | |
} else { | |
return nil, nil | |
} | |
} |
icinga-notifications/internal/config/runtime.go
Lines 92 to 112 in 002d14d
func (r *RuntimeConfig) GetRecipient(k recipient.Key) recipient.Recipient { | |
// Note: be careful to return nil for non-existent IDs instead of (*T)(nil) as (*T)(nil) != nil. | |
if k.ContactID.Valid { | |
c := r.Contacts[k.ContactID.Int64] | |
if c != nil { | |
return c | |
} | |
} else if k.GroupID.Valid { | |
g := r.Groups[k.GroupID.Int64] | |
if g != nil { | |
return g | |
} | |
} else if k.ScheduleID.Valid { | |
s := r.Schedules[k.ScheduleID.Int64] | |
if s != nil { | |
return s | |
} | |
} | |
return nil | |
} |
I'd want to keep that assumption in place as recipient.Key
is used as a key in maps for example and that would result in nasty bugs if recipient.Key
values with more than one member set would ever be used in a map key.
So I think it would be better to add the three columns as explicit members in incident.HistoryRow
if it's now intended that more than one is set to a non-null value.
if !isContact { | ||
groupsOrSchedules[c] = recipient.ToKey(r) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's supposed to happen if the same contact if references multiple times, i.e. directly, via a group and via a schedule? This would just arbitrarily pick a group or schedule. We could write all of that information into the database, but that would probably mean writing multiple rows for the same notification, i.e. the web interface would have to handle and display this probably, so I don't want to decide this on my own.
No matter how we do it, a comment in the schema file how these rows are to be interpreted would be great.
When a contact is notified because of being member of a
Schedule
or aContactGroup
, it may not be clear why a particular contact was notified as it has no direct reference to the current incident. With this PR, noma-web can easily identify that the contact was notified due to a matchingContactGroup
orSchedule
and rendere a proper message accordingly.