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

make sure CLI works for namespaced knex packages as well #2539

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@capaj
Copy link
Contributor

capaj commented Mar 23, 2018

by passing and extra property to liftoff. Without this change when using a namespaced knex like @capaj/knex CLI won't run

@igor-savin-ht

This comment has been minimized.

Copy link
Contributor

igor-savin-ht commented May 23, 2018

@elhigu Is anything missing here?

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented May 23, 2018

I really don't know / understand this change from top of my head :) I suppose it works, but problem with cli changes currently is that we don't have any tests that cli doesn't get broken because of the change. I've been postponing merging any of the cli related PRs, until there are some tests in place.

@capaj

This comment has been minimized.

Copy link
Contributor Author

capaj commented May 23, 2018

@elhigu would be nice to have at least a single happy path test, even if it only calls the CLI without any arguments and asserts that it outputted

  Usage: knex [options] [command]

  Options:

    -V, --version                  output the version number
    --debug                        Run with debugging.
    --knexfile [path]              Specify the knexfile path.
    --cwd [path]                   Specify the working directory.
    --env [name]                   environment, default: process.env.NODE_ENV || development
    -h, --help                     output usage information

  Commands:

    init [options]                         Create a fresh knexfile.
    migrate:make [options] <name>          Create a named migration file.
    migrate:latest                         Run all migrations that have not yet been run.
    migrate:rollback                       Rollback the last set of migrations performed.
    migrate:currentVersion                 View the current version for the migration.
    seed:make [options] <name>             Create a named seed file.
    seed:run                               Run seed files.
Unknown command-line options, exiting

anyway this change makes it possible to namespace knex package and then use this namespaced knex via CLI. Without this change namespaced knex such as @capaj/knex fails on running the CLI.
Maybe you're asking-why should we care if some othe package works or not?
I think it is worth incorporating because it lets contributors easily test changes they make in their own namespaced packages.

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented May 23, 2018

I have nothing against of this change. Its good to support that use case.

Only that I really like to have some test for this (at least that official knex still works fine). I still haven't got time to think how exactly commandline client testsuite should be implemented... there are maybe 3 PRs on hold waiting for those tests...

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 25, 2018

@capaj Considering that we have system for CLI integration tests in place now, do you think it is possible to create some for this PR?

@capaj

This comment has been minimized.

Copy link
Contributor Author

capaj commented Nov 26, 2018

@kibertoad Sure. Let me see if I can get to it this week.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 10, 2018

@capaj Any updates on this :)?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jun 18, 2019

@capaj Status update?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 25, 2019

@capaj To think about it, it's unlikely that tests are necessary here, so this can be merged after rebasement.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 24, 2019

@capaj Any chance you could rebase this?

@capaj capaj force-pushed the capaj:pass-package-name-to-liftoff branch from a2bd3a3 to b33558d Nov 24, 2019
@capaj

This comment has been minimized.

Copy link
Contributor Author

capaj commented Nov 24, 2019

@kibertoad you've got it.

by passing and extra property to liftoff
@capaj capaj force-pushed the capaj:pass-package-name-to-liftoff branch from b33558d to eb90cdc Nov 24, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 25, 2019

Thanks!

@kibertoad kibertoad merged commit e1191ce into knex:master Nov 25, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 27, 2019

Released in 0.20.3

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