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

Use knex to create, run, rollback migrations with samples #24

Conversation

parwatcodes
Copy link
Contributor

@parwatcodes parwatcodes commented Nov 22, 2019

Resolves #21

  • knexfile.js in root is the config file for knex.
  • /src/migrations/ contains 2 migration files for users and tasks table.
  • package.json contains 3 new scripts.
    • make : to create migration files.
    • migrate: to run the migration.
    • rollback: to rollback migration.

@codecov-io
Copy link

codecov-io commented Nov 22, 2019

Codecov Report

Merging #24 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #24   +/-   ##
=======================================
  Coverage   21.02%   21.02%           
=======================================
  Files          11       11           
  Lines         176      176           
  Branches       11       11           
=======================================
  Hits           37       37           
  Misses        137      137           
  Partials        2        2

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 13d9534...b3d5baf. Read the comment docs.

@mesaugat mesaugat added documentation Improvements or additions to documentation example labels Nov 22, 2019
@mesaugat
Copy link
Member

mesaugat commented Nov 22, 2019

@p0k8h Please add a description of what you did in the description section as well.

Example: #7

@mesaugat
Copy link
Member

We usually use imperative mood "spoken or written as if giving a command or instruction" for commits. The commit should explain what the change is.

Read here: https://chris.beams.io/posts/git-commit/

@mesaugat mesaugat changed the title Added instructions for migrations using knex Add instructions for migrations using knex Nov 22, 2019
Copy link
Contributor

@kabirbaidhya kabirbaidhya left a comment

Choose a reason for hiding this comment

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

Looks like you have added here the instructions to create and run migrations but the actual requirement of issue #21 was to have sample tables via knex in the example.

Please setup the migrations and all you need to have in the readme is one line on how to run them.

cc @mesaugat

@kabirbaidhya
Copy link
Contributor

@p0k8h Let's convert all the files added to JavaScript from TS because it looks like the example project was originally based in JS. This is my bad during briefing 😄.

image

@mesaugat
Copy link
Member

@p0k8h Also update the PR title and description.

@parwatcodes parwatcodes changed the title Add instructions for migrations using knex Add Knex config. Scripts to create, run, rollback migrations with samples. Nov 24, 2019
@mesaugat mesaugat changed the title Add Knex config. Scripts to create, run, rollback migrations with samples. Use knex to create, run, rollback migrations with samples Nov 24, 2019
* @returns {Promise<any>}
*
*/

Copy link
Member

Choose a reason for hiding this comment

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

Remove this new line here. Doc blocks should be attached to the method.

*
* @param {Knex} knex
* @returns {Promise<any>}
*
Copy link
Member

Choose a reason for hiding this comment

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

Remove this extra '*'

* @returns {Promise<any>}
*
*/

Copy link
Member

Choose a reason for hiding this comment

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

.

*
* @param {Knex} knex
* @returns {Promise<any>}
*
Copy link
Member

Choose a reason for hiding this comment

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

.

*
* @param {Knex} knex
* @returns {Promise<any>}
*
Copy link
Member

Choose a reason for hiding this comment

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

.

* @returns {Promise<any>}
*
*/

Copy link
Member

Choose a reason for hiding this comment

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

.

* @param {Knex} knex
* @returns {Promise<any>}
*/

Copy link
Member

Choose a reason for hiding this comment

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

.

@parwatcodes parwatcodes force-pushed the sample-tables-using-knex-migrations branch from 7062df4 to 077975c Compare November 25, 2019 04:05
@parwatcodes parwatcodes force-pushed the sample-tables-using-knex-migrations branch from 077975c to fd9cc81 Compare November 25, 2019 04:07
* @param {Knex} knex
* @returns {Promise<any>}
*/
exports.up = function(knex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do export function syntax instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you'll need a stub file for this. cc @mesaugat

Copy link
Contributor Author

@parwatcodes parwatcodes Nov 25, 2019

Choose a reason for hiding this comment

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

Can we do export function syntax instead?

To use es6 syntax we need a transpiler(babel). Shall I add a babel config.

@kabirbaidhya

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's do.

* @param {Knex} knex
* @returns {Promise<any>}
*/
exports.down = function(knex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here and all over.

* @param {Knex} knex
* @returns {Promise<any>}
*/
exports.up = function(knex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

.

* @param {Knex} knex
* @returns {Promise<any>}
*/
exports.down = function(knex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

.

"@babel/node": "^7.7.4",
"@babel/preset-env": "^7.7.4",
"@babel/register": "^7.7.4",
"babel-core": "^6.26.3",
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. You already have included @babel/core.

Copy link
Contributor

@kabirbaidhya kabirbaidhya left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @p0k8h 👍

@kabirbaidhya kabirbaidhya merged commit afd9ba8 into leapfrogtechnology:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation example
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sample tables using knex migrations for the node-app-mssql example
4 participants