Change schedule configuration to support new rotations #204
Conversation
julianbrost
force-pushed
the
schedule-rotations
branch
from
3e03da5
to
b8fc3e4
Compare
|
Force push just updated a comment in |
There was a problem hiding this comment.
I have successfully tested the changes together with the mentioned other PRs. Everything seems to work, even when it is sometimes a bit hard to try, especially due to UI limitations.
I would like to see more debugging logs and, as discussed offline the other day, maybe another handler to the Listener showing the computed schedule. This might also get in handy to compare the calculated rotations against web.
| return contacts | ||
| // GetContactsAt returns the contacts that are active in the schedule at the given time. | ||
| func (s *Schedule) GetContactsAt(t time.Time) []*Contact { | ||
| return s.rotationResolver.getContactsAt(t) |
There was a problem hiding this comment.
For debugging reasons, it would be nice to have a logging message when no Contacts are being returned. This might also happen somewhere up the call stack.
julianbrost
force-pushed
the
schedule-rotations
branch
from
b8fc3e4
to
0ff18c9
Compare
|
Most new changes are within the three new additional commits (please see their commit messages and the updated PR description). Changes to to existing commits should only include changes necessary due to the rebase over #114 (changed import, more |
This adds the required fields and tags to timeperiod.Entry so that this struct can be used directly for querying the database. This removes the previous inline struct TimeperiodEntry that was present in the config loading code. All the initialization is now done in the Init() method instead of when copying the values between the structs. This is done in preparation for changes to schedules where timeperiod entries are loaded directly as part of schedules.
This allows to easily log those using zap.Object().
Reduce the schema file to the minimal changes that are needed to go from the old to the new version. This makes it easier to follow what has been changed and also allows keeping other timeperiods (if they were manually created as it was not yet possible to create those outside of schedules using the web interface so far). Comments were removed from here as they still exist in the full schema file and the purpose of the upgrade file is not to document the schema.
This is a fundamental and incompatible change to how schedules are defined. Now, a schedule consists of a list of rotations that's ordered by priority. Each rotation contains multiple members where each is either a contact or a contact group. Each member is linked to some timeperiod entries which defines when this member is active in the rotation. This commit already includes code for a feature that was planned but is possible using the web interface at the moment: multiple versions of the same rotation where the handoff time defines when a given version becomes active. With this change, for the time being, the TimePeriod type itself fulfills no real purpose and the timeperiod entries are directly loaded as part of the schedule, bypassing the timeperiod loading code. However, there still is the plan to add standalone timeperiods in the future, thus the timeperiod code is kept. More context for these changes: - Icinga/icinga-notifications-web#177 - #193
Lazily initializing the timeperiod entries comes with problems regarding the error handling and would also result in race conditions if multiple callers use the object at the same time. This commit changes this so that Init() has to be called explicitly first, allowing proper error handling and read-only use of the object later.
The most relevant case where this is interesting is when a schedule expands to no contacts at the time of a notification.
julianbrost
force-pushed
the
schedule-rotations
branch
from
0ff18c9
to
0b51dec
Compare
This allows to quickly dump all schedules and how they expand to contacts for the next two days for debugging/testing purposes.
julianbrost
force-pushed
the
schedule-rotations
branch
from
0b51dec
to
d47fde4
Compare
|
Next step: coordinating merging of all the required PRs, that'll be fun. |
resolves #177 **Requirements** * Icinga/ipl-html#137 * Icinga/ipl-web#223 * Icinga/icinga-notifications#204
That was surprisingly easy. |
This is a fundamental and incompatible change to how schedules are defined. Now, a schedule consists of a list of rotations that's ordered by priority. Each rotation contains multiple members where each is either a contact or a contact group. Each member is linked to some timeperiod entries which defines when this member is active in the rotation.
This PR already includes code for a feature that was planned but is not yet possible using the web interface at the moment: multiple versions of the same rotation where the handoff time defines when a given version becomes active.
With this change, for the time being, the TimePeriod type itself fulfills no real purpose and the timeperiod entries are directly loaded as part of the schedule, bypassing the timeperiod loading code. However, there still is the plan to add standalone timeperiods in the future, thus the timeperiod code is kept.
More context for these changes:
The commits in this PR:
Init()on timeperiod entries and require this in other methods (as suggested in Change schedule configuration to support new rotations #204 (comment))./dump-schedulesdebug endpoint that returns how all schedules expand within the next two days (as suggested in Change schedule configuration to support new rotations #204 (review))Dependencies
To allow testing with Icinga Notifications Web, the following PRs are required there:
Caution
Applying the schema upgrade is destructive to the current schedule configuration and there is no easy way to rollback. Be prepared for this and better create a backup first.
closes #193 (schema change already contained in this PR)