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

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

Open
goetas opened this issue Jul 5, 2021 · 8 comments
Open

Comments

@goetas
Copy link
Member

goetas commented Jul 5, 2021

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

  • the customer did not subscribe for a particular feature (thus their db does not have some structures)
  • the migration should be executed only on some envs...
  • more...

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

goetas commented Jul 27, 2021

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

@greg0ire
Copy link
Member

greg0ire commented Aug 1, 2021

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 #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
Copy link
Member Author

goetas commented Aug 2, 2021

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

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

greg0ire commented Aug 2, 2021

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

goetas commented Aug 6, 2021

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

greg0ire commented Aug 6, 2021

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

goetas commented Aug 22, 2021

#1193 is the first step to fix this issue

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

No branches or pull requests

3 participants