yogthos / migratus

MIGRATE ALL THE THINGS!
650 stars 95 forks source link

Base directory is hardcoded for init script #268

Open colinphill-mdsol opened 1 month ago

colinphill-mdsol commented 1 month ago

I'm writing an adapter around Migratus for an internal task runner. I'd like to write tests for it, and I'd like those tests to use a suite of migrations in my project's test_resources directory. I see there is a :parent-migration-dir option which controls where Migratus looks for the migrations directory, and this project's own tests use that option to run test migrations. The init function, however, does not respect this setting; it always looks in resources.

yogthos commented 1 month ago

Ah yeah, that looks like an omission. Any chance you could do a PR for this?

colinphill-mdsol commented 1 month ago

Unfortunately my company's OSS policies are...unsettled, and I can't at present make contributions.

yogthos commented 1 month ago

Ah, fair enough. I'll take a look.

yogthos commented 1 month ago

Hmm, it looks like the init-script should already be resolved relative to the migration directory https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/database.clj#L123

colinphill-mdsol commented 1 month ago

When that function is called, the directory provided does not account for this option. It resolves the :migration-dir option against a base path, but the base path is static.

Tracing the data flow:

We dispatch init on store type, which is generally going to be :database. https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/core.clj#L191-L194

The database implementation of init passes its entire config to get-migration-dir and get-init-script. https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/database.clj#L292-L300

Both of those functions look only at their respective properties, falling back to the defaults of "migrations" and "init.sql". https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/utils.clj#L20-L28

The init-db! function passes those two values directly to find-init-script and slurps the result. https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/database.clj#L279-L282

Nothing along the way has accounted for :parent-migration-dir, and at this point in the call stack we no longer have access to that value because we're no longer holding the entire config.

colinphill-mdsol commented 1 month ago

Here is where the actual hardcoding happens: https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/utils.clj#L80-L89

And the function we've used to find the script does indeed call that unary overload of find-migration-dir: https://github.com/yogthos/migratus/blob/f0972c69669b5832eaf5376fe5a116476923e94d/src/migratus/database.clj#L123-L128

yogthos commented 1 month ago

Ah ok, thanks for digging in. It's been a while since I've looked at it. I'll try get a fix out in the coming days.

colinphill-mdsol commented 1 month ago

Also worth mentioning: This option isn't actually documented. One alternative is to just say it's an implementation detail and not make this change. I think my use case is a compelling one for promoting it to a full-fledged feature, though.

yogthos commented 1 month ago

Yeah, I think this would be worth doing and doesn't look like it should be too much work.

colinphill-mdsol commented 1 month ago

Ah, I just discovered that this also happens with the migrate function. Same basic flow – it lands in that unary overload of find-migration-dir. Full support for this option might wind up touching a fair amount of the public API.

yogthos commented 1 month ago

Ah yeah, looks like it might be a bit more fiddly than it initially appeared. Perhaps for your use case it might be easier to just use with-redefs for find-init-script.

colinphill-mdsol commented 1 week ago

I wound up settling on a workaround of just using a couple .. segments in the configuration. Not ideal, but adequate!

yogthos commented 1 week ago

I'll keep the issue open. I think it'd be good to fix if I get time. Glad the workaround is working in the mean time.