Add notifications for VM destroy, Cloud Volume and Cloud Volume Snapshot actions #85
Conversation
| @@ -8,7 +8,14 @@ module ManageIQ::Providers::Openstack::CloudManager::Vm::Operations | |||
|
|
|||
| def raw_destroy | |||
| raise "VM has no #{ui_lookup(:table => "ext_management_systems")}, unable to destroy VM" unless ext_management_system | |||
| with_provider_object(&:destroy) | |||
| with_notification(:vm_destroy, | |||
| :options => { | |||
There was a problem hiding this comment.
Thanks for the PR, the approach looks good to me.
I think, :instance_name => ... should not be wrapped in :options => ...
There was a problem hiding this comment.
In fact it should - with_notification uses keyword parameter :options not the ruby "options hash".
There was a problem hiding this comment.
Ah, thanks, I forgot about this Ruby 2.0 update.
petrblaho
changed the title
|
This pull request is not mergeable. Please rebase and repush. |
Adds with_notification method witch unifies the usage of Notifications Uses ActiveSupport::Concern with HelperMethods
|
looks like the hash alignment issues are a false positive caused by a nested hash. |
|
I thought that re-running the tests might pass CI, but it looks like it doesn't. Do the notifications need the _success or _error suffix, or are those supposed to be automatically determined? |
|
@tzumainn w/r/t w/r/t failing tests I forgot to add |
Also moves including HelperMethods module to main vm.rb file.
Adds notifications for attach, detach, create, update, and delete actions on Cloud Volume.
|
Checked commits https://github.com/petrblaho/manageiq-providers-openstack/compare/69ed2fa2cd324490532c0f042847711576e7deb2~...f279591d7be2261092ebd70afdb18bd253dacb8c with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 app/models/manageiq/providers/openstack/cloud_manager/cloud_volume/operations.rb
app/models/manageiq/providers/openstack/cloud_manager/cloud_volume_snapshot.rb
|
| @@ -8,6 +8,8 @@ | |||
| config.cassette_library_dir = File.join(ManageIQ::Providers::Openstack::Engine.root, 'spec/vcr_cassettes') | |||
| end | |||
|
|
|||
| NotificationType.seed | |||
There was a problem hiding this comment.
This should be moved to https://github.com/ManageIQ/manageiq/blob/master/lib/evm_database.rb in following PR, no merge blocker. Otherwise LGTM.
|
@petrblaho I did not see this PR before but we should not call Destroy in CF but Delete. |
|
@petrblaho @aufi do you know if it would be possible to backport this and ManageIQ/manageiq#15900 to Fine in order to fix https://bugzilla.redhat.com/show_bug.cgi?id=1524356? |
|
It should be possible IMO (not sure about Seed in spec, but that's just a test). |
|
Marked fine/yes. Needs to be backported for https://bugzilla.redhat.com/show_bug.cgi?id=1524356 |
This adds generic with_notification method to be used to wrap call for fog-openstack actions.