fixes #3624 - raise exception when hook fails

[stbenjam]

Previously, failure of a hook was silent in the GUI: no error bubble. If foreman_hook raises, the error is correctly generated:

I'm not sure if this the erhm, "rubonic" way to do this, though.

[domcleal]

I think we should probably fix this in foreman too, as there might be other orchestration tasks that could fail without raising an exception. What do you think?

[stbenjam]

Well other failures are detected by exceptions in models/concerns/orchestration.rb. I experimented in orchestration.rb to try to find another way to capture the failure + message, but didn't get it to work.

How else would you do it?

[domcleal]

I was thinking about changing this line: https://github.com/theforeman/foreman/blob/develop/app/models/concerns/orchestration.rb#L88

So we do something like:

success = execute :action => task.action
task.status = success ? "completed" : "failed"
failure _("%s task failed") % task.name unless success

Actually having written that, I think this PR is still valuable as it lets us put in a customised error message with the return code too, but a generic catch-all in Foreman itself would be good I think.

[stbenjam]

I see what you mean now. The reason I used the exception was to be able to deliver a message, and that seems how foreman-proxy stuff is doing it. However, exceptions only allow a failure message. There might be a use case to deliver info about a success too?

My first impression of the error handing in orchestration.rb is that it could be improved. Not sure if you're limited by rails or how this works at a deep level, but I like the idea of tasks having a standard way to return. In python I might require all tasks to return status as a tuple (status, message). Maybe the Ruby-way would be to return a hash...{ status => 1, message => "Stuff broke" }.

That would also allow clean-up of this if we could remove ALL the rescues:
https://github.com/theforeman/foreman/blob/develop/app/models/concerns/orchestration.rb#L90-L106

It seems like the logic there belongs in the tasks themselves and for them to return cleaner. But that makes this more complicated, touching lots of different components.

What do you think?

[domcleal]

Sorry for not replying sooner!

I agree, the LeaseConflict exception should definitely be contained inside the DHCP orchestration, the Conflict exception should be a proper return type (like you describe with hashes or tuples) from the orchestration task.

[domcleal]

Merging as-is, because this is still very useful. Thanks @stbenjam!