Skip to content

Add schedule timezone column#344

Merged
nilmerg merged 3 commits intomainfrom
add-schedule-timezone-column
Nov 10, 2025
Merged

Add schedule timezone column#344
nilmerg merged 3 commits intomainfrom
add-schedule-timezone-column

Conversation

@jrauh01
Copy link
Copy Markdown
Contributor

@jrauh01 jrauh01 commented Oct 27, 2025 •
edited by julianbrost
Loading

Add a column to store the timezone the schedule is created for.

Draft until the following PR is merged:

  • Blocked by: Proper timezone handling for schedules icinga-notifications-web#369 (This schema change is solely needed for Notification Web, so no need to merge this earlier than the corresponding PR there. On the other hand, the schema change breaks Notifications Web without that PR as is couldn't insert rows anymore as it doesn't yet know of the required column.)

@jrauh01 jrauh01 self-assigned this Oct 27, 2025
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Oct 27, 2025
oxzi
oxzi reviewed Oct 27, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Schema upgrade files are also required. Please consider some useful default for migration.

Please take a look at #324, where such files are already being created.

@nilmerg
Copy link
Copy Markdown
Member

nilmerg commented Oct 27, 2025

The default an be inferred from the first timeperiod_entry.timezone.

@jrauh01
Copy link
Copy Markdown
Contributor Author

jrauh01 commented Oct 27, 2025 •
edited
Loading

@nilmerg not sure if I understand this right, but the timeperiod_entry.timezone does not have a default.

Edit: Or do you mean to select the timezone value from the first timeperiod_entry that relates to the schedule and use the selected value as default for the migration?

@jrauh01 jrauh01 mentioned this pull request Oct 27, 2025
Merged
@jrauh01 jrauh01 force-pushed the add-schedule-timezone-column branch from 5da9a2d to 7830d9e Compare October 27, 2025 15:22
@nilmerg
Copy link
Copy Markdown
Member

nilmerg commented Oct 27, 2025

select the timezone value from the first timeperiod_entry that relates to the schedule and use the selected value as default for the migration?

Exactly.

@jrauh01 jrauh01 force-pushed the add-schedule-timezone-column branch from 51a9f85 to 1cc5e6d Compare October 28, 2025 09:44
@jrauh01 jrauh01 requested a review from oxzi October 28, 2025 09:45
@julianbrost julianbrost added this to the 0.2.0 milestone Oct 28, 2025
julianbrost
julianbrost reviewed Oct 29, 2025
View reviewed changes
@julianbrost julianbrost mentioned this pull request Oct 29, 2025
Merged
julianbrost
julianbrost reviewed Oct 29, 2025
View reviewed changes
@jrauh01 jrauh01 force-pushed the add-schedule-timezone-column branch from 1cc5e6d to d974095 Compare October 29, 2025 11:06
@jrauh01 jrauh01 requested a review from julianbrost October 29, 2025 11:07
@julianbrost julianbrost marked this pull request as draft October 29, 2025 13:23
julianbrost
julianbrost approved these changes Oct 29, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

This PR is fine and can be merged any time the corresponding Notifications Web PR is ready. Though it probably shouldn't merged earlier as the added column is required without a default, hence without the corresponding change in Notifications Web, older version can't insert new rows anymore.

Thus, I've converted the PR to a draft for the moment (so there's a hint that it maybe shouldn't be merged immediately despite this approval) with a corresponding note in the PR description.

@julianbrost julianbrost mentioned this pull request Nov 5, 2025
Merged
@jrauh01
Add timezone column to mysql schema
724ed4e
@jrauh01
Add timezone column to pgsql schema
dd8fa78
@jrauh01
Add default value for schedule.timezone upgrades
8cf4067 
The value must not be null, so we have to set a default when
upgrading the schema. For that we look for the first
`timeperiod_entry` and use it's `timezone` column. If no entry
exists, the fallback is **'UTC'**.
@jrauh01 jrauh01 force-pushed the add-schedule-timezone-column branch from d974095 to 8cf4067 Compare November 10, 2025 15:35
@nilmerg nilmerg marked this pull request as ready for review November 10, 2025 15:38
@nilmerg nilmerg merged commit 48b79ca into main Nov 10, 2025
26 checks passed
@nilmerg nilmerg deleted the add-schedule-timezone-column branch November 10, 2025 15:45
julianbrost added a commit that referenced this pull request Nov 13, 2025
@julianbrost
Merge schema upgrade files for v0.2.0
cd09f90 
This gives a single file that can be applied during the upgrade more easily,
having it in individual files was mostly to avoid merge conflicts during
development.

The combined files were created with the following command:

  cat 0.2.0-external-uuid.sql <(echo) 0.2.0-source-rules.sql <(echo) 0.2.0-schedule-timezone.sql <(echo) 0.2.0-source-username.sql > 0.2.0.sql

The order of files matches the order in which the PRs that added them were
merged (though the order shouldn't really matter as there are no dependencies
between the individual files):

 - #216
 - #324
 - #344
 - #338
@julianbrost julianbrost mentioned this pull request Nov 13, 2025
Merged
julianbrost added a commit that referenced this pull request Nov 13, 2025
@julianbrost
Merge schema upgrade files for v0.2.0
cf58325 
This gives a single file that can be applied during the upgrade more easily,
having it in individual files was mostly to avoid merge conflicts during
development.

The combined files were created with the following command:

  cat 0.2.0-external-uuid.sql <(echo) 0.2.0-source-rules.sql <(echo) 0.2.0-schedule-timezone.sql <(echo) 0.2.0-source-username.sql > 0.2.0.sql

The order of files matches the order in which the PRs that added them were
merged (though the order shouldn't really matter as there are no dependencies
between the individual files):

 - #216
 - #324
 - #344
 - #338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julianbrost julianbrost julianbrost approved these changes

@oxzi oxzi Awaiting requested review from oxzi

Assignees

@jrauh01 jrauh01

Labels

cla/signed CLA is signed by all contributors of a PR

Projects

None yet

Milestone

0.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jrauh01 @nilmerg @julianbrost @oxzi