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

Migrations up/down commands: filename parameter #3416

Merged
merged 7 commits into from Oct 6, 2019

Conversation

@ivanovych666
Copy link
Contributor

ivanovych666 commented Aug 26, 2019

Add the ability to explicitly define the migration's filename for up and down commands (#1550).

Current CLI

Run only the next migration:

migrate:up

Undo only the latest migration:

migrate:down

Proposed CLI:

Add the optional name parameter for up and down commands.

Run only the specified or the next migration:

knex migrate:up <name>

Undo only the specified or the latest migration:

knex migrate:down <name>

Example 1:

Let's say we have two new migrations:

001_one.js
002_two.js

If we want to run only the 002_two.js then we can use this:

knex migrate:up 002_two.js

Example 2:

Let's say we have two completed migrations:

001_one.js
002_two.js

If we want to revert only the 001_one.js then we can use this:

knex migrate:down 001_one.js
bin/cli.js Outdated
.description(' Undo the last migration performed.')
.action(() => {
.command('migrate:down [<name>]')
.description(' Undo the last or the defined migration performed.')

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

specified is likely to be clearer than defined

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done

bin/cli.js Outdated
.action(() => {
.command('migrate:up [<name>]')
.description(
' Run the next or the defined migration that has not yet been run.'

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

specified is likely to be clearer than defined

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done

bin/cli.js Outdated
.description(' Undo the last migration performed.')
.action(() => {
.command('migrate:down [<name>]')
.description(' Undo the last or the defined migration performed.')

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

that was already run might be more consistent with woring of up command

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done

this.config.migrationSource,
all,
completed
).slice(0, 1);
);
const migrationToRun = name

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 26, 2019

Collaborator

Can you also throw an error if name was specified but migration was not found?

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done, throws an error in up and in down methods

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 26, 2019

Thanks! Can you also create a documentation PR at https://github.com/knex/documentation?

ivanovych666 pushed a commit to ivanovych666/documentation that referenced this pull request Aug 27, 2019
Copy link
Contributor Author

ivanovych666 left a comment

@kibertoad , I have created a documentation PR, it is here knex/documentation#233

bin/cli.js Outdated
.description(' Undo the last migration performed.')
.action(() => {
.command('migrate:down [<name>]')
.description(' Undo the last or the defined migration performed.')

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done

bin/cli.js Outdated
.description(' Undo the last migration performed.')
.action(() => {
.command('migrate:down [<name>]')
.description(' Undo the last or the defined migration performed.')

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done

this.config.migrationSource,
all,
completed
).slice(0, 1);
);
const migrationToRun = name

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

done, throws an error in up and in down methods

const migrationFile1 = '001_one.js';

return assertExec(
`node ${KNEX} migrate:down ${migrationFile1} \

This comment has been minimized.

Copy link
@kibertoad

kibertoad Aug 27, 2019

Collaborator

assertExecError is a better way to test negative scenarios

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Aug 27, 2019

Author Contributor

Thank you, you are right. Done.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 28, 2019

@ivanovych666 Tests are failing now 😹

@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Aug 30, 2019

@kibertoad , (I hope) I have fixed integration tests by adding null as the first argument in up and down calls. But if we need to make this backward compatible, then in these methods we can use something like this:

if (!config && name && typeof name === 'object') {
    config = name;
    name = null;
}

What do you think about this?

@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Sep 20, 2019

@kibertoad Is there any chance to have this pull request approved? If there is something more that I can do then please let me know.

@@ -169,7 +190,7 @@ class Migrator {
});
}

down(config) {
down(name, config) {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Sep 22, 2019

Collaborator

can't the name be part of config to keep this change backwards-compatible?

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Sep 23, 2019

Author Contributor

done

--connection=${temp}/db \
--migrations-directory=${migrationsPath}`,
'run_migration_002'
).then(({ stdout }) => {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Sep 22, 2019

Collaborator

it is preferable to use async/await syntax

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Sep 23, 2019

Author Contributor

done

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 3, 2019

Collaborator

was this change pushed?

This comment has been minimized.

Copy link
@ivanovych666

ivanovych666 Oct 6, 2019

Author Contributor

You are right, thank you for mention this, I forgot to do this, now this is done. Please, let me know if there anything else.

Copy link
Collaborator

kibertoad left a comment

Current implementation is not backwards-compatible due to the introduction of the new parameter.

@ivanovych666 ivanovych666 force-pushed the ivanovych666:issues-1550 branch from cce21a6 to a7ceeab Sep 23, 2019
@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Sep 23, 2019

I have updated the pull request as per comments above.

@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Oct 3, 2019

Is there anything else that I can do here?

@ivanovych666 ivanovych666 force-pushed the ivanovych666:issues-1550 branch from 45360f3 to 134b59f Oct 6, 2019
@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Oct 6, 2019

@kibertoad , I'm not sure, but it looks like there is one test failed which should not be related to this pull request. I have made a rebase of my PR on your master, but this didn't help. Do you have any ideas on how to solve this?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

I think this is related to just released PG 12, since we are using the latest version. I'll fix it separately.

@kibertoad kibertoad merged commit 7fabae9 into knex:master Oct 6, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
kibertoad added a commit to knex/documentation that referenced this pull request Oct 6, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

@ivanovych666 New CLI test is failing in master: object tested must be an array, a map, an object, a set, a string, or a weakset, but undefined given

Could you please fix it in a separate PR?

ivanovych666 added a commit to ivanovych666/knex that referenced this pull request Oct 6, 2019
@ivanovych666

This comment has been minimized.

Copy link
Contributor Author

ivanovych666 commented Oct 6, 2019

Oops, sorry for that, new PR is ready here #3466

@ivanovych666 ivanovych666 deleted the ivanovych666:issues-1550 branch Oct 6, 2019
kibertoad added a commit that referenced this pull request Oct 6, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

Released in 0.19.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.