search

opengisch / pum

Postgres Upgrades Manager
GNU General Public License v2.0
30 stars 7 forks source link

use file name instead of checksum to check whether a migration was run #81

Closed olivierdalang closed 2 years ago

olivierdalang commented 4 years ago

As per https://github.com/opengisch/pum/issues/79

This is a draft, not sure of the approach... would probably be worth keeping checksum verification to display warnings.

3nids commented 4 years ago

it makes sense, no objection on my side

olivierdalang commented 4 years ago

The test failed because in the test suite, there are cases where two migrations have the same file name, but are in a different folders.

With the old logic, both run, because they had different checksums anyways. With the new logic, only the first runs, as the second time, it's considered already applied.

Is there a proper use case for two identically named files in different dirs representing different migrations ? Is it used somewhere ? This could not be supported anymore as there no way to properly distinguish the folders (sounds like a bad idea anyways :-) ) .

So, with this PR, the migrations are identified using version, description, type, where any two files producing the same values would be considered run already.

3nids commented 4 years ago

I believe this is fine. This might be worth adding some notes in the README. Shall I merge?

olivierdalang commented 4 years ago

@3nids

Shall I merge?

From a strictly-QGEP perspective, yes, but I'm not really aware of usage outside of QGEP.

haubourg commented 4 years ago

From a strictly-QGEP perspective, yes, but I'm not really aware of usage outside of QGEP.

I'd ask @elemoine here (hi Eric!)

elemoine commented 4 years ago

Thanks for notifying me @haubourg!

No strong opinion here.

One thing though: this docstring in the code suggests to use a date in the form ddmmyy as the delta name. And there may be other places making the same suggestion. If __is_applied is changed to rely on (version, name, type) rather than (version, checksum) we certainly want to suggest giving unique names to deltas.

ponceta commented 2 years ago

@olivierdalang status on this? This was a good or a bad idea considering your experiences?

olivierdalang commented 2 years ago

I think the change is good for the reason described in the initial ticket.

From a QGEP perspective, we can merge, because the case of two different migrations with the same name does not happen in QGEP. Maybe if we do that, we should completely remove the ability to specify multiple delta directories (for which I don't know the use case anyway), so that name clashes in migrations are not possible anymore.

(however I can't tell if it's worth putting effort in this currently)

olivierdalang commented 2 years ago

Still slightly scared, but we got to merge it at some point... As far as I understood, the only know case of multi delta dirs is for sigip, where there's just a custom pre and post-all.

So merging and let's hope for the best :-)