search

theforeman / foreman_hooks

Run custom hook scripts on Foreman events
http://m0dlx.com/blog/Extending_Foreman_quickly_with_hook_scripts.html
GNU General Public License v3.0
56 stars 50 forks source link

Task name is now unique and post queue mechanism #57

Closed lzap closed 6 years ago

lzap commented 6 years ago

There was a change recently in core so orchestration task names must be unique. This was added to detect multiple tasks added twice. If a hook is named file.sh and it is in multiple hook directories, it is always added as "Hook: file.sh" so the hook is only added once and rest are always ignored.

This patch includes full filename path, task name is slightly longer but it prevents from ignoring hooks.

I am also adding another commit into this PR - post queue orchestration mechanism. I hope that README explains that all. This needs more testing but generally it is a good idea to let users to hook via post_queue.

lzap commented 6 years ago

@orrabin this should help

ShimShtein commented 6 years ago

ACK from me, parameters are shown only in postcreate hook, and no update hooks fired.

lzap commented 6 years ago

@sean797 to give you some context, IHAC who don't see nested parameters and all_parameters in host create hooks, @ShimShtein identified this is an ActiveRecord "reload" issue when references are not yet available in around_save. Luckily our orch framework also has post_queue which triggers after_commit which fixes the issue. This creates two new hooks, for delete this is not relevant.

lzap commented 6 years ago

Downstream BZ is: https://bugzilla.redhat.com/show_bug.cgi?id=1615372

lzap commented 6 years ago

Updated the rake task.

This line does not make sense to me:

@obj.exec_hook(@filename, @event == 'destroy' ? 'create' : @event, *args)

Because few lines up it actually makes sure that method is never called when event is destroy:

queue_to_use.create(:name => "Hook: #{filename}", :priority => priority.to_i,
  :action => [HookRunner.new(filename, self, event.to_s), event.to_s == 'destroy' ? :hook_execute_del : :hook_execute_set])

So that never fires. Removed as useless code.

lzap commented 6 years ago

Well I wish we had some tests but hopefully this does not break things. Thanks for quick review!