Set time_zone to London in all GOV.UK apps #381
Conversation
Our conventions for rails apps say that we should set time_zone to London: https://docs.publishing.service.gov.uk/manual/conventions-for-rails-applications.html#specify-london-as-the-timezone In practice however, only about half of the apps do so. Based on the apps running in integration: Apps which set the time_zone to London: AccountApi Collections CollectionsPublisher ContentPublisher Collections Frontend GovernmentFrontend Static EmailAlertApi Frontend GovernmentFrontend GovukChat LocationsApi Maslow PlacesManager Publisher ReleaseApp SearchApiV2 Signon SmartAnswers SpecialistPublisher Static Whitehall Apps which leave the time_zone as it's default UTC: AuthenticatingProxy Contacts ContentDataAdmin ContentPerformanceManager ContentStore ContentTagger ContentStore RouterApi EmailAlertFrontend Feedback FinderFrontend HmrcManualsApi LinkCheckerApi LocalLinksManager ManualsPublisher RouterApi SearchAdmin SearchV2Evaluator ServiceManualPublisher ShortUrlManager Support SupportApi Transition TravelAdvicePublisher Having the apps use inconsistent time_zones feels like a recipe for trouble. config.time_zone has to be set before active_support.initialize_time_zone runs, so we specify that our initializer must run before it. Unfortunately this means that there's no way for applications to set their own value of time_zone. I think this is probably desirable for consumers of govuk_app_config though - I can't think of a legitimate reason why we'd want some apps using different time_zones. Technically this is a breaking change, as it could change behaviour in apps which are using timezone-specific methods and expecting to get UTC times out. Hopefully we're already explicit about when we want UTC and when we want local time though. Once this is merged, I'll remove the bit in the docs about setting time_zone, as it will be automatic.
There was a problem hiding this comment.
Seems sensible to standardise this in all the apps.
A scenario where it could be an issues is if we depend on some external process that runs in UTC but I also can’t think of where it would be an issue within application.
Irrelevant (because we are using cron schedule for it) example:
In Content Data API an import has to run (currently 40 minutes) after a GoogleAnalytics query has run and GA schedule is in UTC.
The Rails/TimeZone cop should also keep things in check.
The more I think about it, the more I worry that having the gem forcibly set time_zone to London without any warnings could lead to a confusing / hard to debug problem down the line. For example, someone trying to set config.time_zone explicitly to UTC, or trying to set it to something else entirely, and then being confused when it's still London. There's no way I can think of to distinguish the "time_zone is UTC because that's the default" situation from the "time_zone is UTC because it's been set explicitly" situation. Logging an info message at least increases the chance that someone will notice what's going on if they are trying to set it to UTC. We're also logging the situation where time_zone is set to London - this config is fairly widespread, and it will become redundant following this change to the gem. However it's harmless, so I don't think it's worth chasing up all the other edge cases. All other values of time_zone should be considered errors, as it would be very confusing to have them silently overridden.
richardTowers
dismissed stale reviews from AgaDufrat and MuriloDalRi
Pushed substantial changes in a subsequent commit
|
👍🏻 Thanks Aga and Murilo.
Yeah - it's also still possible to refer to UTC times explicitly, if that's what the code needs. Which we should be doing anyway hopefully. If we wanted to do like a sidekiq schedule thing (which I appreciate isn't our preferred approach, instead favouring k8s cronjobs) that had to run daily at a particular UTC time, that would still be fine. Having thought about it a little more, I was worried about the possibility of this causing confusion if someone is trying to set time_zone to something else explicitly, so I've made it log what its doing and error in the situation where someone's trying to do something the gem is going to undo. This meant I had to change the tests quite a bit, so I think it'll need a re-review I'm afraid. |
|
@richardTowers LGTM! |
Our conventions for rails apps say that we should set time_zone to London:
https://docs.publishing.service.gov.uk/manual/conventions-for-rails-applications.html#specify-london-as-the-timezone
In practice however, only about half of the apps do so. Based on the apps running in integration:
Apps which set the time_zone to London:
Apps which leave the time_zone as it's default UTC:
Having the apps use inconsistent time_zones feels like a recipe for trouble.
config.time_zonehas to be set beforeactive_support.initialize_time_zoneruns, so we specify that our initializer must run before it. Unfortunately this means that there's no way for applications to set their own value oftime_zone. I think this is probably desirable for consumers ofgovuk_app_configthough - I can't think of a legitimate reason why we'd want some apps using different time zones.Technically this is a breaking change, as it could change behaviour in apps which are using timezone-specific methods and expecting to get UTC times out. Hopefully we're already explicit about when we want UTC and when we want local time though.
Once this is merged, I'll remove the bit in the docs about setting time_zone, as it will be automatic.