search

frioux / DBIx-Class-DeploymentHandler

https://metacpan.org/pod/DBIx::Class::DeploymentHandler
20 stars 25 forks source link

`deploy` should not run `__VERSION` #50

Closed Altreus closed 5 years ago

Altreus commented 7 years ago

prepare_install runs prepare_deploy and then prepare_version_storage_install, which implies the concept of deployment does not include the concept of version storage install.

It follows that deploy should skip the __VERSION DDL, or the __VERSION DDL should not be in the same directory as the 001st deploy - and then install itself should first discover the __VERSION DDL and then run the first deployment.

The current setup makes it impossible to have multiple schemata in the same database. I've created a HandlesVersionStorage implementation that stores the class name of the schema being installed as well as the version, but I can't use it, because every schema contains a __VERSION DDL and so I can't currently install the second one.

Altreus commented 7 years ago

I can "fix" it with an extra line in __ddl_consume_with_prefix, to skip __VERSION DDL.

https://github.com/Altreus/DBIx-Class-DeploymentHandler/commit/99150390e7c7f8603cc15b8a9b0ce022d7beaf67

The bad thing is, this is not discovered from version_rs->result_source->source_name, because this DeployMethod object isn't given that information.

The good thing is, this DeployMethod class is already hard-coding __VERSION instead of trying to look the name up dynamically, so there's prior art to doing it improperly.

https://github.com/frioux/DBIx-Class-DeploymentHandler/blob/master/lib/DBIx/Class/DeploymentHandler/DeployMethod/SQL/Translator.pm#L598

However, if the DeployMethod object can get to the version RS somehow, both of these lines should be fixed.

mohawk2 commented 5 years ago

@Altreus Where are we with this? What would you like DH do? So long as it still passes its tests (ie back-compat), I'll be happy to accommodate.

Altreus commented 5 years ago

I think this became an architectural discussion that would be best put on the list of things to consider in a new version. I managed to refactor my module to not require this change any more.

I don't think this would be backwardly compatible because people may already be using these methods as they are, and they're the public API.

With both of those in mind I'm just going to close it :)