search

tj / node-migrate

Abstract migration framework for node
MIT License
1.54k stars 224 forks source link

`migrate foo` invalid command is not an error #157

Closed hdon closed 5 years ago

hdon commented 5 years ago

Giving migrate an invalid command does not issue an error and exits normally:

hdon@ubuntu-s-1vcpu-1gb-sfo2-01:~/migrate-project$ npm i migrate
npm WARN saveError ENOENT: no such file or directory, open '/home/hdon/migrate-project/package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open '/home/hdon/migrate-project/package.json'
npm WARN migrate-project No description
npm WARN migrate-project No repository field.
npm WARN migrate-project No README data
npm WARN migrate-project No license field.

+ migrate@1.6.2
added 20 packages from 13 contributors and audited 21 packages in 1.15s
found 0 vulnerabilities

hdon@ubuntu-s-1vcpu-1gb-sfo2-01:~/migrate-project$ ./node_modules/.bin/migrate init
  migrations dir : /home/hdon/migrate-project/migrations
hdon@ubuntu-s-1vcpu-1gb-sfo2-01:~/migrate-project$ ./node_modules/.bin/migrate make first_migration
  migration : complete
hdon@ubuntu-s-1vcpu-1gb-sfo2-01:~/migrate-project$ echo $?
0

Note: migrate make first_migration is not a valid migrate command.

wesleytodd commented 5 years ago

Hi @hdon, make is not a command in this tool. To see how it works I suggest using migrate --help. Good luck!

LinusU commented 5 years ago

@wesleytodd I think that the issue was that migrate didn't raise an error, and exited with a 0 status code, instead of raising an error and setting a failure status code :relaxed:

Opening for now, but feel free to close if I'm wrong 🐎

wesleytodd commented 5 years ago

Right, but this is related to the other issue, because what it is running is the "default" command up, but make is not a valid migration. So because previously that was not an error case it would not exit with an error code.

That being said, I am not sure we have good test coverage of exit codes, so that might be a good area to look at.

hdon commented 5 years ago

@LinusU is correct.

I chose the example of "make" because it's the exact mistake I made to discover the confusing behavior.

@wesleytodd so in addition to not erroring, it also migrated all the way up? :scream:

wesleytodd commented 5 years ago

Yeah, so the fix is to error when you try to run with a migration name which does not exist. We can close this in favor of #156, because it is really the same issue.

hdon commented 5 years ago

Yeah, that would be better. Though IMHO it's better for tools to ask you to be more explicit. I think defaulting to "up" is not really saving anyone any effort but some will get tripped up by the automatic "up" behavior.

wesleytodd commented 5 years ago

@hdon I agree, maybe we should also change that behavior. That was also the pre-1.0 behavior. Want to open a new issue to make that recommendation?