Skip to content

Conversation

olivierdalang
Copy link
Contributor

As per #79

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

to check whether a migration was run
@3nids
Copy link
Member

3nids commented Apr 28, 2020

it makes sense, no objection on my side

@olivierdalang
Copy link
Contributor Author

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.

@olivierdalang olivierdalang marked this pull request as ready for review July 29, 2020 17:09
@olivierdalang olivierdalang changed the title [draft] use file name instead of checksum to check whether a migration was run use file name instead of checksum to check whether a migration was run Jul 29, 2020
@3nids
Copy link
Member

3nids commented Sep 2, 2020

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

@olivierdalang
Copy link
Contributor Author

@3nids

Shall I merge?

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

@haubourg
Copy link

haubourg commented Sep 2, 2020

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
Copy link
Contributor

elemoine commented Sep 2, 2020

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
Copy link

ponceta commented May 5, 2022

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

@olivierdalang
Copy link
Contributor Author

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)

AND success = 'TRUE'
""".format(
self.upgrades_table, delta.get_version(), delta.get_checksum())
self.upgrades_table, delta.get_version(), delta.get_name(), delta.get_type())
Copy link
Member

Choose a reason for hiding this comment

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

if the other PR gets through, let's do formatted strings here too :)

@olivierdalang olivierdalang merged commit a73c249 into opengisch:master Aug 11, 2022
@olivierdalang
Copy link
Contributor Author

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 :-)

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.

Upgrader.__is_applied code is not robust (we shouldn't use checksums)
5 participants