doctrine / migrations

Doctrine Database Migrations Library
https://www.doctrine-project.org/projects/migrations.html
MIT License
4.68k stars 389 forks source link

Skipped migrations do not play well with `doctrine:migrations:up-to-date` #1179

Open goetas opened 3 years ago

goetas commented 3 years ago

When there is one or more skipped migrations, doctrine:migrations:up-to-date will always return a non-zero exit code.

We are using doctrine:migrations:up-to-date to understand if there are migrations left to execute while deploying new code to production (only after all migrations are executed we run the deploy process). As soon as there is at lest one skipped migration, the doctrine:migrations:up-to-date command becomes useless...

Skipping a migration has multiple reasons (that should not block a deployment):

This is tricky and the only solution I have in mind is to insert in the migrations table also the skipped migrations, and later the doctrine:migrations:up-to-date --allow-skipped-migrations command will consider skipped migrations as "done" (and return a zero exit code) (--allow-skipped-migrations is a new parameter to maintain BC)

goetas commented 3 years ago

TBH I do not know what is the expected behavior here... :(

greg0ire commented 3 years ago

doctrine:migrations:up-to-date will always return a non-zero exit code.

Why? Because the migration is considered new by the status calculator? Or because of some less obvious condition? I don't know how it works, sorry…

From what you tell us though, I would expect that command not to fail on a skipped migration. As established in https://github.com/doctrine/migrations/issues/1164, there is nothing wrong with skipping a migration.

@ogizanagi, maybe you can be of some advice here too, since it seems you use that feature?

goetas commented 3 years ago

is considered new by the status calculator

yes, exactly. since the migration has not been executed (skipped), when checking if there are new migrations, the command returns non-zero exit code.

But here IMO the situation is tricky... I personally do not know what is the right solution here. Since also skipped migrations can be executed (because the condition for which they have been skipped is not valid anymore as example), in that case is correct that doctrine:migrations:up-to-date returns a non zero exit.

ogizanagi commented 3 years ago

We use skipped migrations for migrations that should run only in some envs, or on a specific database when the application has multiple connections. So it seems obvious to me this command should not fail on skipped migrations ; these are not errors.

If there is an actual use-case for this to fail, we could introduce a --fail-on-skipped-migrations flag in the command later to cover this, but to me the current situation should be considered as a "bug" in regard of the meaning of skipped migrations (to me, but looking a bit for history of this feature, it's not that obvious to everybody what was this feature meant for).

Since also skipped migrations can be executed (because the condition for which they have been skipped is not valid anymore as example), in that case is correct that doctrine:migrations:up-to-date returns a non zero exit.

If the reason for which it was skipped is not valid anymore, then the migration is not to be skipped, thus returning a non-zero exit code. If we decide to fix the return code for skipped migration, it won't affect this anyway.

greg0ire commented 3 years ago

I'm not currently using this package, but I agree a lot what @ogizanagi said. I also strongly feel that whatever behavior we pick, it should be heavily documented, to make it clear what the contract is.

goetas commented 3 years ago

The issue is that we do not know if a migration is skipped or not until we do not run the migrate command. Skipped migrations are considered new migrations. doctrine:migrations:up-to-date returns a non zero exit code because it seems that those migrations have to be executed...

so it seems a recursive problem. to me, since skipped migrations can be executed at any time (different envs, user configs or whatever), is near to impossible to determine if doctrine:migrations:up-to-date returned non zero because there are skipped migrations or because there are new migrations.

I do not think that is a solutions, but it might help to save in the migrations table data when a migration gets skipped, and later build commands/tools to handle that situation.

greg0ire commented 3 years ago

it might help to save in the migrations table data when a migration gets skipped

Yes absolutely. Once a migration has been skipped on a given server, it shouldn't be possible to not skip it if not by removing a record in the table IMO.

goetas commented 3 years ago

https://github.com/doctrine/migrations/pull/1193 is the first step to fix this issue