Wrap errors in _error key #5
Conversation
Previously, the task helper would rescue errors in the `run` method and simply print their hashified version. This caused Bolt to synthesize its own `_error` object (simply stating the task failed and its exit code) which took precedence over the actual underlying cause. We now properly wrap the result in `_error` so Bolt knows that it should display that information to the user.
nicklewis
requested review from
lucywyman and
a team
and removed request for
lucywyman
|
This seems like a great change, though it'll also need a 1.0 release. I don't think that will be a problem? |
|
The beauty of semver is that pre-1.0 there are no rules! And honestly I would call this a bug. It's a bug that a user or two may rely on, but it's pretty unambiguously a bug. |
|
I guess I don't see the harm in doing a 1.0. The risk is that there are people who rely on it who this breaks, and even if it doesn't break the letter of the semver law it feels like it'll give people a bad day. That said I guess folks will have to pin in their puppetfile either way, so maybe it doesn't matter? |
| exit 1 | ||
| rescue StandardError => e | ||
| error = TaskHelper::Error.new(e.message, e.class.to_s, e.backtrace) | ||
| STDOUT.print(error.to_h.to_json) | ||
| STDOUT.print({ _error: error.to_h }.to_json) |
There was a problem hiding this comment.
Would it be worth adding a failure status? That way with human outputter there is not an empty hash under the error message.
| STDOUT.print({ _error: error.to_h }.to_json) | |
| STDOUT.print({status: 'failure', _error: error.to_h }.to_json) |
cas@cas-ThinkPad-T460p:~/working_dir/bolt$ bolt task run test::ruby_task_helper name=hi -t localhost
Started on localhost...
Failed on localhost:
ooops task failed
{
"status": "failure"
}
Failed on 1 node: localhost
Ran on 1 node in 0.19 sec
There was a problem hiding this comment.
@lucywyman @nicklewis I think we agree on the version. What do ya'll think about the status key?
There was a problem hiding this comment.
I don't like the status key because we already define success/failure as part of the task spec. Also, there is no status key in the success case, which mean it's not something you can actually rely on.
If the problem is a superfluous empty hash being printed after the error message, we could fix that in bolt rather than changing the result here.
Previously, the task helper would rescue errors in the run method and simply print their hashified version. This caused Bolt to synthesize its own _error object (simply stating the task failed and its exit code) which took precedence over the actual underlying cause. We now properly wrap the result in _error so Bolt knows that it should display that information to the user. Porting over the changes from puppetlabs/puppetlabs-ruby_task_helper#5
|
I ported over this update to the python task helper (and included the I agree this is a bug that does not need a |
Previously, the task helper would rescue errors in the run method and simply print their hashified version. This caused Bolt to synthesize its own _error object (simply stating the task failed and its exit code) which took precedence over the actual underlying cause. We now properly wrap the result in _error so Bolt knows that it should display that information to the user. Porting over the changes from puppetlabs/puppetlabs-ruby_task_helper#5
Previously, the task helper would rescue errors in the
runmethod andsimply print their hashified version. This caused Bolt to synthesize its own
_errorobject (simply stating the task failed and its exit code) whichtook precedence over the actual underlying cause.
We now properly wrap the result in
_errorso Bolt knows that it shoulddisplay that information to the user.