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

Re-enable alpha1 meaningful message when "latest" already reached. #988

Merged
merged 7 commits into from
Jun 18, 2020

Conversation

bugalot
Copy link
Contributor

@bugalot bugalot commented Jun 14, 2020

Q A
Type improvement
BC Break no
Fixed issues #987

Summary

Tries to fix the issue described in issue #987. To summarize, it is a proposal to get back the nice and clean "[WARNING] Already at latest version" when trying to perform a migration on a platform already at "latest" version.

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.

Thanks for your contribution.

I'm ok with this change, I've left only one suggestion

@bugalot
Copy link
Contributor Author

bugalot commented Jun 14, 2020

I implemented the suggested change. The message will be now like "Already at version "VERSION_ALIAS" ("VERSION")".

Thanks a lot for the review @goetas !

@bugalot bugalot requested a review from goetas June 14, 2020 16:27
@jmsche
Copy link

jmsche commented Jun 16, 2020

Hi,

I'm just wondering if it makes sense to throw a warning/error if all migrations have been executed?

Also throwing a warning while passing the --allow-no-migration option seems weird to me as we explicitly allow it (eg. during deploy).

And thank you @bugalot for the message improvement :)

@elkuku
Copy link

elkuku commented Jun 17, 2020

Not sure why this isn't considered a BC break since it breaks CI without passing the (new) --allow-no-migration option :(

Please merge soon ;)

@greg0ire
Copy link
Member

@elkuku what makes you say it is not considered a BC-break? This is version 3.0.0…

@greg0ire
Copy link
Member

I'm just wondering if it makes sense to throw a warning/error if all migrations have been executed?

If all migrations have been executed, running the migrations command is a bit weird, and notifying the user about it makes sense IMO.

Also throwing a warning while passing the --allow-no-migration option seems weird to me as we explicitly allow it (eg. during deploy).

Good point, maybe it fits more the description for notice: https://github.com/php-fig/log/blob/master/Psr/Log/LoggerInterface.php#L82

@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/master, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@goetas goetas merged commit a5c63dc into doctrine:master Jun 18, 2020
@goetas
Copy link
Member

goetas commented Jun 18, 2020

Thanks!

goetas added a commit to goetas/migrations that referenced this pull request Jun 20, 2020
Re-enable alpha1 meaningful message when "latest" already reached.
@greg0ire greg0ire modified the milestones: 3.1.0, 3.0.1 Jun 21, 2020
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.

5 participants