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

[up-to-date] print the names of executed unavailable migrations #886

Closed
wants to merge 1 commit into from
Closed

Conversation

dbu
Copy link
Member

@dbu dbu commented Dec 3, 2019

Q A
Type improvement
BC Break no
Fixed issues -

Summary

I was confused why the up-to-date command does not tell which migrations are missing. From the logic in this command, we can't tell so at least we can point to the status command which will output that information. As we have the list available, print it.

(Looking at the code, there is a scenario where i have an additional migration that should be done, and have a migration in the database that has been deleted in the filesystem. If those 2 numbers are the same, the up-to-date command will incorrectly state that all is up to date. This should be improved but i consider it out of scope for this small improvement here.) (i had been looking at an outdated version of the command)

@alcaeus alcaeus requested a review from goetas December 3, 2019 13:37
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!
$executedUnavailableMigration contains the list on unavailable migrations, does it make sense to list them already here or just to redirect users to use the status command?

@dbu dbu changed the title hint status command when up-to-date check finds non-existing migrations print migration names when up-to-date check finds non-existing migrations Dec 4, 2019
@dbu dbu changed the title print migration names when up-to-date check finds non-existing migrations [up-to-date] print the names of executed unavailable migrations Dec 4, 2019
@dbu
Copy link
Member Author

dbu commented Dec 4, 2019

in our project we use an old version of doctrine/migrations, that confused me into writing all these comments - sorry. i now simply print the list of executed unavailable migrations. does that make sense like this? (i decided to go shorter than the migrate command which lists a line for each such migration, and prints the date if available - if you think i should do exactly the same as there, i can adjust it)

@dbu
Copy link
Member Author

dbu commented Dec 4, 2019

fixed the code style issue

@goetas
Copy link
Member

goetas commented Dec 4, 2019

The list of unavailable migrations could be quite long.

Another point is that we have already a command to list the not available migrations migrations:status --show-versions... my question here is: is this the right command to show again the list of missing migrations?
Considering that we are already displaying the count of them...

@dbu I'm sorry for making you work in advance, I should have raised my concerns before...

@dbu
Copy link
Member Author

dbu commented Dec 5, 2019

we use the migration up-to-date check in our CI. it would be convenient to not have to log onto the server and run another command (and having to figure out what command to run) to see what migration the status command is talking about.

my original PR was making the message at least print the name of the other command you need.

if your concern is mainly the length of the list, i can also print just the first 3 or so, and then say "and X more." if there are more than 3. i guess there is essentially 2 cases: somebody tinkered with migrations and one file somehow was removed after having been run, or no migration files have been found. in the first case it would be helpful to see the migration(s) so you can check what happened with that one. in the second case printing does not help - you need to fix something to make migrations see the files again, or truncate the migrations_versions table because you rolled up all migrations into one file or something like that.

@goetas
Copy link
Member

goetas commented Dec 6, 2019

Your motivation convinced me!

Regarding the "long list of missing migrations": what about using a Table as in

?

@goetas goetas added this to the 3.0.0 milestone Dec 6, 2019
@dbu
Copy link
Member Author

dbu commented Dec 6, 2019

haha, and your worry made me realize that the full list is unlikely to ever be useful. i'd rather cut off after 3 versions. would you agree with that?

@alcaeus
Copy link
Member

alcaeus commented Dec 6, 2019

  • Using an extra command for this can certainly be helpful
  • The list can cary depending on the verbosity setting, e.g. nothing on --quiet, up to 3 or 5 migrations by default, and full list on -v. WDYT?

@goetas
Copy link
Member

goetas commented Dec 6, 2019

I would suggest to display all the missing migrations, but only if some extra option is specified as suggested by Andreas. Without that command will keep the output as it is

@dbu
Copy link
Member Author

dbu commented Jan 7, 2020

@alcaeus @goetas so which should it be? i like the suggestion of @alcaeus better - okay if i adjust to that?

@goetas
Copy link
Member

goetas commented Jan 8, 2020

I'm not sure if an extra command here is the solution... we have already maybe too many of the.
What should this command be named?
Consider also that I plan to implement #734 in v3, maybe it can be just displayed there?

@dbu
Copy link
Member Author

dbu commented Jan 8, 2020

ups sorry, i misread the suggestion. i was thinking of keeping it on the up-to-date command, but doing the verbosity thing as alcaeus proposed.

i agree that we should not add more commands for this.

my use case is that an automated check would immediately print what is wrong, instead of only that "something" is wrong. for that use case, the output would need to be on the up-to-date command. if you think its not worth having that, i have no problems if you refue this pull request - i consider it useful, but its not super important as i can run the status/list command after seeing the failure.

regarding number of commands, you could have status --check or something like that and deprecate up-to-date, but that would be implicit and contrary to creating that new list command.

@goetas
Copy link
Member

goetas commented Jan 8, 2020

I think that this PR adds value, so I would like to see it implemented.

I'm just concerned if is worth having some --quiet option to display more or less stuff. I would just place a Table with all the missing migrations, and that's it. Less options and special cases we have to manage, the better.

@alcaeus
Copy link
Member

alcaeus commented Jan 10, 2020

I can live without a quiet switch - this can always be added in the future if necessary. 👍 from my side.

@goetas
Copy link
Member

goetas commented Jan 15, 2020

alternative draft in #917

@dbu
Copy link
Member Author

dbu commented Jan 15, 2020

closing in favor of #917

@dbu dbu closed this Jan 15, 2020
@dbu dbu deleted the patch-2 branch January 15, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants