fedora-infra / fas

Fedora Account System
https://admin.fedoraproject.org/accounts
GNU General Public License v2.0
40 stars 50 forks source link

Miscellaneous fixes #193

Closed pypingou closed 7 years ago

laxathom commented 8 years ago

:+1: from me. The utc thing works from a scratch fas3's db and a fas2's migration my side. Need to validate this on staging though.

pypingou commented 8 years ago

The amount of changes has increased quite a bit there ^^

pypingou commented 8 years ago

One remark, otherwise looks sane

skrzepto commented 8 years ago

login still doesnt work. try logging into fasadmin, the first time it works, the second time it fails with the same error as i reported.

I think the UTCDateTime implementation was actually correct

laxathom commented 8 years ago

hm...lemme guess, you're using an SQLite engine, right? I know that SQLite has a lot of caveat on datetime and sqlalchemy provides its own datetime impl for SQLite datetime formatting.

skrzepto commented 8 years ago

i think your right. we should then remove sqlitte from the *.ini files so no one accidently comes across these issues and force postgres as the dev db

link to src http://docs.sqlalchemy.org/en/latest/core/type_basics.html#sqlalchemy.types.DateTime.__init__

skrzepto commented 8 years ago

Just confirmed this works in postgresql. If we just make a note in the readme about using postgres only then I'm okay with this PR.

skrzepto commented 7 years ago

@laxathom @pypingou

Are we deciding to stick with postgres for dev and prod db? This will solve the tz aware issue that is caused by sqlite.

pypingou commented 7 years ago

I think we should stick to postgres yes

@laxathom thoughts?

laxathom commented 7 years ago

:+1: here. However, let's not close the door to other DBAPI if people come around with a way to support them (MySQL, Oracle and the like)

skrzepto commented 7 years ago

@laxathom I think I read somewhere that MySQL has the same issues with storing timezone aware as sqlite. We can look at https://pypi.python.org/pypi/SQLAlchemy-UTCDateTime/ just not sure if we want to do that or not. Thoughts?

laxathom commented 7 years ago

@skrzepto let's open a discussion ticket to check this so we can move forward, merge and close this PR

skrzepto commented 7 years ago

lgtm as well

pypingou commented 7 years ago

Let's merge then :)