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

Run a range of migrations #32

Merged
merged 2 commits into from May 12, 2022
Merged

Run a range of migrations #32

merged 2 commits into from May 12, 2022

Conversation

winklertomas
Copy link
Contributor

@winklertomas winklertomas commented Apr 29, 2022

Motivation

Which issue does this fix? Fixes issue

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

Try to run a range of migrations

@winklertomas winklertomas requested a review from a team April 29, 2022 08:58
@Simply007
Copy link
Contributor

It might be worth describing the argument information in README

  • mention what happened with status.json
  • cover status.json update by test

@winklertomas
Copy link
Contributor Author

winklertomas commented May 6, 2022

I've added unit tests to validate the order, however the status.json update itself, in my opinion, doesn't require more coverage. I have similar feelings about the README edits, I've updated it a bit and added an example into the help section, but the new concept of running migrations from a specified range nicely complements the current implementation. It modifies the status.json in the same way, it just writes each migration to status.json to avoid running it again.

I've also added a validity check for the migration order, the current implementation already checks for duplicates, so I think it will be possible to close #33 as well. I don't want to check for example that there is no gap in the migration order, it would make deleting migrations unnecessarily difficult.

I will add an example of running a range of migrations to the boilerplate as well

Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Overall seems OK.
Just a couple of notes -> none of them is super important, but the DOM in tsconfig.json

tsconfig.json Show resolved Hide resolved
const from = Number(match[1]);
const to = Number(match[2]);

return from <= to ? [from, to] : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I might return

{
  from,
  to
}

object

Just for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've introduced new IRange interface, thanks for the tip

README.md Show resolved Hide resolved
src/tests/invalidOrderMigration.test.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

LGTM - the if/else forest in run.ts might be refactored, but this is not a part of the task and it is still fairly readable.

@winklertomas winklertomas merged commit 71f2ea8 into master May 12, 2022
@winklertomas winklertomas deleted the Run-a-range-of-migrations branch May 12, 2022 09:10
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.

Run a range/set of migration
2 participants