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

Fix exit code migrate cli #151

Merged
merged 2 commits into from Jan 14, 2014

Conversation

Projects
None yet
4 participants
@vvo
Contributor

vvo commented Jan 14, 2014

The knex command line for executing migration tasks was not exiting properly (exit code).

Thus when running with grunt, it says everything is ok even if migrations failed.

I did not see any tests about the cli command line.

vvo added some commits Jan 14, 2014

process.exit() with proper statusCode
One thing to take into account when making command lines
is to exit with the proper code so that grunt-tasks works as expected
when failing.
@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Jan 14, 2014

Two answers to this. Your code is 100% correct. Good catch, and thanks.

You might want to think about refactoring your migrations though. The API is pretty stable—I fixed a bunch of the bugs about a month back. (https://github.com/tgriesser/knex/blob/master/lib/migrate.js)

Rather than run the CLI from Grunt, you'd be better off just defining a custom task and using the API. If you really wanted to, you could write a plugin that would pass in config defined in your Gruntfile.

bendrucker added a commit that referenced this pull request Jan 14, 2014

Merge pull request #151 from vvo/fix-exit-code-migrate-cli
Migrate CLI should exit with code 1 if the migration fails

@bendrucker bendrucker merged commit 8480c0f into tgriesser:master Jan 14, 2014

1 check passed

default The Travis CI build passed
Details

@vvo vvo deleted the vvo:fix-exit-code-migrate-cli branch Jan 14, 2014

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 14, 2014

Thank you, could you publish a patch to npm?

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Jan 14, 2014

I'm going to defer to @tgriesser on npm publishing.

In the mean time, I'd highly recommend using your forked git repo directly in your package. That's what I do for bookshelf and knex — it makes it easy to write a quick patch and have it accessible immediately. It also means you are merging in updates in git rather than updating your package. It gives you some extra control that can be really useful.

This will certainly land on the NPM registry soon, but it's good overall practice to use Git for any package you want to modify.

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 19, 2014

@tgriesser can you publish the various patches to npm anytime soon ? Thank you.

@bendrucker We used to do what you said: use a fork inside our application.

But current npm and npm shrinkwrap are buggy (1, 2).

I won't get into details but, as soon as you start putting some github urls inside your packages dependencies, future installs using regular npm versions will fails to update.

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Jan 19, 2014

I can take a look at recent hotfixes and figure out what can be rolled into a release later today.

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 28, 2014

Guys, you should definitely publish the new patched version.

Creating a patched version is easy as two steps and will fix knex for many people.

I do not get why you are waiting for more patches to publish a new version.
Each time you merge pull requests you should publish a patched version right away.

Unpublished code is dead code.

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Jan 28, 2014

It's not that anyone's waiting for more patches. Tim's been busy and he's the one with publish rights. Nothing is dead and if you're going to depend on the public npm registry a little patience is appreciated.

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 28, 2014

Too bad there's only one people with npm push rights, giving people push rights is easy too if you need it.

thank you.

@johanneslumpe

This comment has been minimized.

Collaborator

johanneslumpe commented Jan 28, 2014

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Jan 28, 2014

Yeah, been pretty busy and just didn't get around to updating the documentation yet to push a new version release, I'll do it now (I was looking to get something in there about the vagrantfile but I'll do that later)... an exit code just didn't seem like that critical of an issue to rush out the door.

Also, you can always specify the commit sha in your package.json if it's really a blocker.

dependencies: {
   "knex": "git://github.com/tgriesser/knex.git#05efb708d1c75c75363029b3669eb89e9d25039e"
}
@vvo

This comment has been minimized.

Contributor

vvo commented Jan 28, 2014

Yes @tgriesser like @bendrucker said but npm is so much buggy theses times that as soon as
you put a git commit url in your package.json and if you are using npm-shrinkwrap well
it does not end well.

The current exit code issue can corrupt your whole database.
When there's an error in your migration scripts,
any grunt task/jenkins process will say "Everything's fine, go on" which is dangerous.

Also, yes it seems not critical but the steps to publish a new version are very easy and should not take more than 30 seconds, so why not just do it right away and fix the issue, this is what I did not get.

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented Jan 28, 2014

There's more to it than npm publish. There's the changelog, running tests, etc. @tgriesser just sent you some ideas for an automated build/deploy process that could actually run in one command and then do most of the heavy lifting on Travis.

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 28, 2014

Hmm I see tests are not run automatically on travis-ci. Could be a great PR to do.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Jan 28, 2014

Yeah, the tests are run on travis. Also, most of that is covered with grunt release... it was actually writing up docs around the new vagrantfile that's in the repo that was delaying me.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Jan 28, 2014

@vvo

This comment has been minimized.

Contributor

vvo commented Jan 28, 2014

Thanks a lot,

btw I would have used the git commit trick if it worked well. We used to and we stopped since
it was too buggy in production mode with shrinkwrap.

So sad that npm has flaws right now.

elliotf pushed a commit to elliotf/knex that referenced this pull request Nov 24, 2014

elliotf pushed a commit to elliotf/knex that referenced this pull request Nov 24, 2014

elliotf pushed a commit to elliotf/knex that referenced this pull request Nov 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment