Deploy Provider's migration command should display output on failure

We're running into this too... Especially frustrating since when we ssh into the machine and run rake db:migrate manually (using copied and pasted command from error output), the command does not fail.

To me, it seems like :output_on_failure => true should be the default setting. I can't think of when I wouldn't want to see output from a failed command.

The first thing I always want to know when a command failed is what the output from the command was, to help me understand why it would have failed.

IMHO, it's very important to either output or at least *save to a file* any command output, in case of failure.

If you try to run the command again manually to see what went wrong, you may get completely different output than the initial failure would have told you.

This is particularly true for running Rails migrations.

For example, if a migration fails near the end of the migration the first time you run it, and then you run it again, it will likely fail at the very column that it attempts to added, with an error such as Mysql2::Error: Duplicate column name 'col2': ALTER TABLE `table` ADD `col2` decimal(5,2) AFTER `col1`, aborting *earlier* than it did the 1st time it was run, and requiring painstaking manual inspection of the tables (describe table; ) to figure out how much of the migration actually successfully ran the first time through.

Workaround for now is to manually edit gems/chef-0.10.8/bin/../lib/chef/mixin/command.rb on the server (a bad practice, I know):

if Chef::Log.level == :debug # || opts[:output_on_failure]
output << "\n---- Begin output of #{opts[:command]} ----\n"
output << command_output.to_s
output << "\n---- End output of #{opts[:command]} ----\n"
end
raise Chef::Exceptions::Exec, "#{opts[:command]} returned #{status.exitstatus}, expected #{opts[:returns]}#{output}"

I submitted a pull request with the small modification that I made locally to the deploy resource to test out a migration failure that I was seeing.

https://github.com/opscode/chef/pull/457

It just passes "output_on_failure => true" like @Grégory Karékinian suggested.

We've got a set of functional tests for the deploy provider coming shortly in PR #468 which would allow us to switch this to mixlib-shellout and have good enough test coverage to tell if that causes any regressions. That would be preferable because otherwise we're playing whack-a-mole making this change throughout the provider.

We'll probably look at this again on Wednesday and see where we are at.

The functional tests have been merged so we should be confident in a switch to mixlib-shellout now. Doing so will provide the desired functionality as well as increase stability all over.