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

Migration messaging improvements #6622

Merged
merged 1 commit into from
Mar 22, 2016
Merged

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Mar 21, 2016

This builds on #6621 and ended up being a little more involved than I had originally intended. To start with I just added a flag to logInfo so that it would output the message differently - but the code was instantly harder to read and the tests became very hard to reason about.

Plus, I know, really, we need to move towards having proper 'loggers' as per #2001 and this uses the right sort of pattern.

So.. instead of passing a single logInfo method around the migration system, we now pass around a logger object which has an info and a warn method. They do similar things - prefixing a message with either 'Migrations:' or 'Skipping Migrations:' and adding a colour. The normal message is blue, the warning is yellow.

Everywhere in the data & fixture migrations where a migration is skipped over, a warning message is now being output.

So, if I run a simple 003->004 migration, I get output like this, where you can see I didn't have any badly formed tags:

And then if I use FORCE_MIGRATION=true npm start to get the migrations to run a second time, I get this:

And we can easily see that none of the migrations actually ran.

The aim here is just to make debugging easier & make things a little more explicit and clear. Hopefully we'll reap the benefits later 😁

refs #6301

  • fix messages that joined with comma and therefore missed outputting version no
  • change logInfo to logger that has both an info and a warn method
  • add new warn method to errors
  • add a warn message every time a migration (data or fixture) gets skipped over
  • update logger everywhere, including tests
  • update tests to check logger.warn gets called

refs TryGhost#6301

- fix messages that joined with comma and therefore missed outputting version no
- change `logInfo` to `logger` that has both an info and a warn method
- add new warn method to errors
- add a warn message everytime a migration (data or fixture) gets skipped over
- update logger everywhere, including tests
- update tests to check logger.warn gets called
@ErisDS ErisDS force-pushed the migration-messaging branch from 0342a4d to 84f3877 Compare March 22, 2016 10:00
sebgie added a commit that referenced this pull request Mar 22, 2016
@sebgie sebgie merged commit 229c292 into TryGhost:master Mar 22, 2016
@ErisDS ErisDS deleted the migration-messaging branch March 22, 2016 14:25
ErisDS added a commit to ErisDS/Ghost that referenced this pull request Mar 24, 2016
refs TryGhost#6621, TryGhost#6622

- remove unneeded `return new Promise.resolve()` lines
- reduce code in tests
- improve quality of tests checking that all task functions are executed
- add missing test coverage
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

Successfully merging this pull request may close these issues.

2 participants