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

Basic Migrations command #494

Merged
merged 5 commits into from
Nov 2, 2017
Merged

Basic Migrations command #494

merged 5 commits into from
Nov 2, 2017

Conversation

XVincentX
Copy link
Member

@XVincentX XVincentX commented Oct 31, 2017

This pull request will introduce a migration mechanism for the gateway leveraging node-migrate@next module by Tj.

Connects #490
Connects #492 closes #492

Standing points:

  • This module can track the ran migrations using a .migrate file or using a custom storage mechanism. Question is whether we want to use the file or store the things in Redis, for example.
    If we keep them on Redis, you move the DB, you are ok. If you keep them on the file system you might re-run the migrations if you change the server. ✌️

  • How do we make sure the users will run the migrations? I've checked Kong and they do not care. They rely on the fact that you read the change log and notice you've got a migration to run; therefore you run them. I'd stick with the same mechanism ✌️

  • Figure out a way to test the effects by this migration
    Integrate the migration command with the eg cmd line (we're not doing that)

@XVincentX
Copy link
Member Author

// @kevinswiber @DrMegavolt What's your idea around the standing points?

@DrMegavolt
Copy link
Contributor

what is wrong with just running migrate ? why do we need eg cli integration ?

@ExpressGateway ExpressGateway deleted a comment from codecov bot Nov 1, 2017
@XVincentX
Copy link
Member Author

I thought it was a requirement in order to give to the user an uniform interface to use the gateway.
Anyway, npx migrate or execute the bin file directly will do the job perfectly and it'll also save me some work. I'm fine removing this 👍

@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #494 into master will increase coverage by 0.01%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage      92%   92.01%   +0.01%     
==========================================
  Files         219      221       +2     
  Lines        9280     9333      +53     
==========================================
+ Hits         8538     8588      +50     
- Misses        742      745       +3
Impacted Files Coverage Δ
...9389756098-credential-username-userid-transform.js 91.3% <91.3%> (ø)
...migrations/credential-username-userid-transform.js 96.66% <96.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 097fb93...750a419. Read the comment docs.

@XVincentX
Copy link
Member Author

I removed the CLI integration and we'll stick with the default storing mechanism! 👍

Copy link
Contributor

@DrMegavolt DrMegavolt left a comment

Choose a reason for hiding this comment

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

LGTM

@XVincentX
Copy link
Member Author

@DrMegavolt Please review this one ore time. I've modified the code to use the dao instead of the service — this is to skip the password hashing and have a simple password copy. In this way our users won't have to communicate any new password.

Also, I've added a test that should check the whole flow.

let tmpFile;
let userId;
let oldCredential;
before('I insert some credentials by user ID', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the name should be
I insert some credentials by username

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah!

Copy link
Contributor

@DrMegavolt DrMegavolt left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge

@XVincentX XVincentX merged commit d6e5f9d into master Nov 2, 2017
@XVincentX XVincentX deleted the feature/migrations branch November 2, 2017 10:55
gatherchou pushed a commit to yilu-tech/express-gateway that referenced this pull request Jul 29, 2021
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.

Baisc migrations support
3 participants