Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqitch add suddently uses same template for all engines while add.all = true #795

Closed
Cielquan opened this issue Oct 19, 2023 · 1 comment · Fixed by #796
Closed

sqitch add suddently uses same template for all engines while add.all = true #795

Cielquan opened this issue Oct 19, 2023 · 1 comment · Fixed by #796
Assignees
Labels

Comments

@Cielquan
Copy link

Context

I recently started using sqitch for my DB migration management. I use MySQL for production and SQLite for quick testing. I use sqlfluff for linting and autoformatting my SQL scripts.

Because sqlfluff complains about BEGIN in MySQL Scripts, which is used in the sqitch default template for MySQL, I added custom templates to use START TRANSACTION in MySQL Scripts.

What I want

I want sqitch to create scripts for both SQLite and MySQL from there respective templates when I run e.g. sqitch add test -n "test".

What I get

When I run sqitch add test -n "test" I correctly get 6 test.ddl Scripts like shown in the file tree below. But the content of those files is the problem. At random sqitch chooses to either use MySQL or SQLite templates for both engines. The examples below are from the case MySQL templates were used.

I already did 2 migrations with sqitch before today and I added the templates before the first migration. So the setup worked 2 times already. So I am really confused why it suddenly stopped working.

I also tried checking out the commit where I added the templates and running sqitch add test -n "test", but I get the same bogus result.

Version

I installed sqitch around 7 weeks ago (according to git) and never updated it, but v1.4.0 is the most recent so it should not matter.

$ sqitch --version
sqitch (App::Sqitch) v1.4.0

OS

ubuntu 22.04.1

sqitch.conf

[core]
	top_dir = migrations
[engine "mysql"]
	target = db:mysql:
	registry = migration_data
	top_dir = migrations/mysql
	plan_file = migrations/sqitch.plan
	extension = ddl
[engine "sqlite"]
	target = db:sqlite:
	registry = migration_data
	top_dir = migrations/sqlite
	plan_file = migrations/sqitch.plan
	extension = ddl
[target "project"]
	uri = db:mysql:project
[target "project-sqlite"]
	uri = db:sqlite:project.db
[deploy]
	verify = true
[rebase]
	verify = true
[add]
	all = true
	template_directory = migrations/templates
[tag]
	all = true
[rework]
	all = true
[bundle]
	all = true

Truncated file tree in my repo:

sqitch.conf
migrations
├── sqitch.plan
├── mysql
│   ├── deploy
│   │   └── test.ddl
│   ├── revert
│   │   └── test.ddl
│   └── verify
│       └── test.ddl
├── sqlite
│   ├── deploy
│   │   └── test.ddl
│   ├── revert
│   │   └── test.ddl
│   └── verify
│       └── test.ddl
└── templates
    ├── deploy
    │   ├── mysql.tmpl
    │   └── sqlite.tmpl
    ├── revert
    │   ├── mysql.tmpl
    │   └── sqlite.tmpl
    └── verify
        ├── mysql.tmpl
        └── sqlite.tmpl

Templates

migrations/templates/deploy/mysql.tmpl

-- Deploy [% project %]:[% change %] to [% engine %]
[% FOREACH item IN requires -%]
-- requires: [% item %]
[% END -%]
[% FOREACH item IN conflicts -%]
-- conflicts: [% item %]
[% END -%]

start transaction;

-- XXX Add DDLs here.

commit;

migrations/templates/deploy/sqlite.tmpl

-- Deploy [% project %]:[% change %] to [% engine %]
[% FOREACH item IN requires -%]
-- requires: [% item %]
[% END -%]
[% FOREACH item IN conflicts -%]
-- conflicts: [% item %]
[% END -%]

begin;

-- XXX Add DDLs here.

commit;

migrations/templates/revert/mysql.tmpl

-- Revert [% project %]:[% change %] from [% engine %]

start transaction;

-- XXX Add DDLs here.

commit;

migrations/templates/revert/sqlite.tmpl

-- Revert [% project %]:[% change %] from [% engine %]

begin;

-- XXX Add DDLs here.

commit;

migrations/templates/verify/mysql.tmpl

-- Verify [% project %]:[% change %] on [% engine %]

start transaction;

-- XXX Add verifications here.

rollback;

migrations/templates/verify/sqlite.tmpl

-- Verify [% project %]:[% change %] on [% engine %]

begin;

-- XXX Add verifications here.

rollback;

Migration files

migrations/mysql/deploy/test.ddl

-- Deploy swlp:test to mysql

start transaction;

-- XXX Add DDLs here.

commit;

migrations/mysql/revert/test.ddl

-- Revert swlp:test from mysql

start transaction;

-- XXX Add DDLs here.

commit;

migrations/mysql/verify/test.ddl

-- Verify swlp:test on mysql

start transaction;

-- XXX Add verifications here.

rollback;

migrations/sqlite/deploy/test.ddl

-- Deploy swlp:test to sqlite

start transaction;

-- XXX Add DDLs here.

commit;

migrations/sqlite/revert/test.ddl

-- Revert swlp:test from sqlite

start transaction;

-- XXX Add DDLs here.

commit;

migrations/sqlite/verify/test.ddl

-- Verify swlp:test on sqlite

start transaction;

-- XXX Add verifications here.

rollback;
@theory theory self-assigned this Oct 21, 2023
@theory theory added the bug label Oct 21, 2023
@theory
Copy link
Collaborator

theory commented Oct 21, 2023

Yay, someone is using templates sufficiently to surface this bug! Confirmed it is a bug, mostly because it was never updated to support adding change scripts to multiple engines at once, which was implemented all the way back in 0c7237c.

theory added a commit that referenced this issue Oct 21, 2023
When adding a change to multiple engines, Sqitch would randomly pick one
engine's templates or the others and create them all for the same
engine. In other words, adding a change to both "sqlite" and "pg"
targets would result in all change files generated from templates from
only one of those engines -- whichever it happened to find first.

Due to the use of a hash reference for templates in add.pm. Fix it by
making a copy of the hash instead. This leads to duplication of effort
when multiple targets use the same engine, but the cost is generally low
relative to how often `add` is called -- and likely is measured in
milliseconds.

Resolves #795.
theory added a commit that referenced this issue Oct 21, 2023
When adding a change to multiple engines, Sqitch would randomly pick one
engine's templates or the others and create them all for the same
engine. In other words, adding a change to both "sqlite" and "pg"
targets would result in all change files generated from templates from
only one of those engines -- whichever it happened to find first.

Due to the use of a hash reference for templates in add.pm. Fix it by
making a copy of the hash instead. This leads to duplication of effort
when multiple targets use the same engine, but the cost is generally low
relative to how often `add` is called -- and likely is measured in
milliseconds.

Resolves #795.
@theory theory closed this as completed in 0702d9a Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants