ENHish: fixes #849 - NOT READY TO MERGE

[wasade]

Python patches are tracked separately, and an N-1 python patch can be applied. What this means practically is that, if a python patch fails, patching execution stops. When a patch is attempted again, the patch system will check if there is a python patch that was not applied (that corresponds to the present SQL patch level), and apply it.

This is not ideal however. This requires a patch to the settings table. The settings table is not part of the qiita schema though. As a result, this table is not dropped on clean_test. What this means is that patches to the settings table cannot be done through the patch system, and either require a different mechanism or hard coded patches. This PR is doing the latter as the former is very not fun, and it is not clear if there is a benefit from the effort required to appropriately resolve the logic in the face of a production system. The other option I see is to move the settings table into the qiita schema and allow it to be dropped. Open to other suggestions here though.

[ElDeveloper]

There are legitimate failures in the build.

[coveralls]

Coverage increased (+0.24%) to 78.58% when pulling 912188d on wasade:issue_849 into d398414 on biocore:master.

[ElDeveloper]

table -> table is

[ElDeveloper]

The other option I see is to move the settings table into the qiita schema and allow it to be dropped. Open to other suggestions here though.

I think this is the correct path to follow and avoid the inline patch that you currently have in your PR.

[ElDeveloper]

Code looks good otherwise!

[ElDeveloper]

👍 ... sorry for the triple email.

[antgonza]

Agree with @ElDeveloper, patch is better than "fixing" on code

[josenavas]

My only question is if such table can go into the qiita schema... I remember that there was some good reasons to keep it outside, but I don't remember right now. @adamrp @squirrelo do you remember?

[squirrelo]

We left it outside so that the database could constantly have a marker of if it was a test database or not, independent of the actual schema. This way if the schema dropped it would still be marked as a test database or not.

[adamrp]

@squirrelo exactly; also, IIRC running tests drops the qiita schema between
tests, and wouldn't want to drop the settings table.

On Thu, Feb 26, 2015 at 10:34 PM, Joshua Shorenstein <
notifications@github.com> wrote:

We left it outside so that the database could constantly have a marker of
if it was a test database or not, independent of the actual schema. This
way if the schema dropped it would still be marked as a test database or
not.

—
Reply to this email directly or view it on GitHub
#860 (comment).

[wasade]

It sucks so much having this separate but as long as we basically never need to modify this table, then we're fine. If we need to do additional modifications, then I think we may want to revisit whether we need to have the settings table separate, and if so, what other strategies might be apprioriate

[squirrelo]

Since we are tracking non-static things that change as the schema is dropped, can we have a generic settings table and a qiita.settings table that holds data specific to the schema and instance? That way we don't lose the safety system of having a settings table outside the schema, but also aren't doing schema-specific things outside the schema.

[wasade]

Is this still relevant or can it be closed?

[josenavas]

This is taking care by #1318 since the python patch is executed in the same transaction as the SQL path so if it fails the SQL path is also rollbacked.

[squirrelo]

Can this be closed due to #1318 being merged?

[josenavas]

Correct, this is no longer needed