-
Notifications
You must be signed in to change notification settings - Fork 2.8k
UTC data migration doesn't work in Linux (closes #20002) #20112
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
base: v17/dev
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This pull request fixes a Linux compatibility issue in UTC data migration where IANA time zone IDs couldn't be used with SQL Server's AT TIME ZONE operator. The solution converts IANA IDs to Windows IDs for SQL Server while maintaining SQLite compatibility.
Key Changes
- Removed validation that only accepted system-available time zones
- Enhanced migration logic to handle IANA to Windows time zone ID conversion
- Restructured the migration to support both SQLite and SQL Server with appropriate time zone handling
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
SystemDateMigrationSettingsValidatorTests.cs | Deleted test file for removed validator |
MigrateSystemDatesToUtc.cs | Enhanced migration logic with IANA/Windows timezone conversion and database-specific handling |
UmbracoBuilder.Configuration.cs | Removed registration of deleted validator |
SystemDateMigrationSettingsValidator.cs | Deleted validator that restricted timezone IDs to system-available ones |
src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs
Outdated
Show resolved
Hide resolved
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.
Thanks @lauraneto. This looks good to me from a code perspective (at least once you've considered the minor comments I left). Once you are happy with it in your testing, please can you also liaise for next week with @andr317c and @nikolajlauridsen to verify this does now work as expected now on SQL Linux and any other variations we need to support (e.g. SQL Azure)? Thanks.
src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs
Outdated
Show resolved
Hide resolved
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.
Couldn't quite follow why it was necessary to remove this validation. Seems we still call TimeZoneInfo.FindSystemTimeZoneById
in the migration, so if so, it might be good to validate this upfront when Umbraco boots?
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.
I removed it because it doesn't really matter if the application recognizes the time zone or not (at least when using SQL Server), what matters is if the SQL Server itself recognizes it.
Edit:
Just to add, this is helpful for edge cases where your OS differs from the one running the database, or if you don't have have all timezones installed on your docker image for example.
For SQLite we validate it only at the time of use, during the migration.
Co-authored-by: Andy Butland <[email protected]>
Closes #20002
This pull request adjusts the UTC migration code so that if a local time zone id is in the IANA format, it is converted to its corresponding Windows Id.
This is required for SQL Server as the AT TIME ZONE implementation relies on a Windows mechanism to convert datetime values across time zones, and therefore only accepts Windows ids.