Skip to content

migrate timestamp fields to timestamptz #1223

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

Merged
merged 2 commits into from
Dec 27, 2020

Conversation

syphar
Copy link
Member

@syphar syphar commented Dec 26, 2020

with some time on my hands I started on #1067 with the migration to timezone aware datetime fields.
In general I believe it's a good idea to be explicit about timezones also on the database level.

As we see, it simplifies quite some code :)

I'm happy to apply any changes needed (the column defaults actually don't have to be changed, since NOW() and CURRENT TIMESTAMP already are timezone aware)

Tests work locally, not sure what else needs to be tested.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

r? @Nemo157

@jyn514 jyn514 added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Dec 27, 2020
@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Dec 27, 2020
@jyn514 jyn514 merged commit 78e822c into rust-lang:master Dec 27, 2020
@syphar syphar deleted the timestamp-timezone branch December 27, 2020 18:19
@syphar
Copy link
Member Author

syphar commented Dec 28, 2020

@jyn514 @Nemo157 not sure who does deploy this,

while working on another issue I ran into a panic in rustwide_builder caused by this change, seems like that part is not covered with tests.

I'll build a quick fix for that

@syphar
Copy link
Member Author

syphar commented Dec 28, 2020

first I have to check if that's related to me running the builder in the web container

@syphar
Copy link
Member Author

syphar commented Dec 28, 2020

forget what I said, likely my mistake. The web Dockerfile copies the local release build to the container, which likely was outdated.

Just building and checking

@jyn514 jyn514 removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants