ASKBOT / askbot-devel

Askbot is a Django/Python Q&A forum. **Contributors README**: https://github.com/ASKBOT/askbot-devel#how-to-contribute. Commercial hosting of Askbot and support are available at https://askbot.com
Other
1.56k stars 627 forks source link

Fix issue #845 and put some effort into postgres migrations #852

Closed martin-bts closed 4 years ago

martin-bts commented 4 years ago
evgenyfadeev commented 4 years ago

Hey the fix on 845 here is good, I will it as you suggest, however I think won't be merging this PR, explaining the reason below.

While it's ok to make migrations running postgres scripts that no longer apply (only if you're 100% sure) into a migration that just returns and does nothing, it is probably not a good idea to delete the files retroactively as there probably are sites that ran those migrations. To accept this I'd need a proof that this change won't break the migrations on the existing files.

+1 on deleting unused files.

I'll see if I can quickly reproduce this without changing the migration dependencies and in that case I will close this PR without merging it.

martin-bts commented 4 years ago

I do not know anything about Django migrations and I am unable to handle them. I am glad somebody else provided up to date migrations in https://github.com/ASKBOT/askbot-devel/pull/853 because I have been trying to reproduce these and Django would always tell me that it doesn't see any model changes. Therefore I cannot provide any proof for or against anything. We can keep the empty migrations if that makes any sense and only remove the unused files and adapt the current migrations to be Postgres 11+ compatible.

evgenyfadeev commented 4 years ago

I have incorporated the "DROP AGGREGATE IF EXISTS concat_tsvectors(tsvector);" in 6fd75c54470ced7f91d1077bd25d293a9b81974a it's nice that you've stumbled on this. This makes the script compatible with older versions of postgres and version 12.

Closing this as the issue of repeated scripts is now fixed.