Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds new NotificationTypes - VM destroy, Cloud Volume and Cloud Volume Snapshot actions #15900

Merged
merged 3 commits into from Oct 23, 2017

Conversation

petrblaho
Copy link

In order to have actions in OpenStack provider create Notifications we need new NotificationTypes for them. Associated PR for OpenStack provider is ManageIQ/manageiq-providers-openstack#85

@miq-bot miq-bot added the wip label Aug 28, 2017
@petrblaho petrblaho changed the title [WIP] Adds new NotificationTypes Adds new NotificationTypes - VM destroy, Cloud Volume and Cloud Volume Snapshot actions Sep 22, 2017
@miq-bot miq-bot removed the wip label Sep 22, 2017
@skateman
Copy link
Member

@miq-bot assign @skateman

@petrblaho
Copy link
Author

This is needed for ManageIQ/manageiq-providers-openstack#85

@skateman
Copy link
Member

@petrblaho is there any reason in not using the subject where possible?

@petrblaho
Copy link
Author

@skateman thank you for your review and chat help. Looking into how to use that now.

@petrblaho petrblaho force-pushed the add-notifications branch 2 times, most recently from 2c84aad to 63dda1d Compare October 18, 2017 16:22
@tzumainn
Copy link
Contributor

@skateman hi! could you take a look at petr's updates and see if this looks good now?

@Loicavenel
Copy link

@skateman Please Help!!!

@@ -174,3 +174,83 @@
:expires_in: 24.hours
:level: :error
:audience: global
- :name: vm_destroy_success
:message: 'Destroying Instance %{subject} completed successfully.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the instance here is a good word if you name the notification as vm_*

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok, I can change that to VM or Virtual Machine.

:level: :error
:audience: global
- :name: cloud_volume_create_success
:message: 'Creating Volume %{volume_name} completed successfully.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why no subject here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b/c this called when creating that resource - and we do not have it so we cannot pass it into notification.

I can use subject name as it would just have volume_name value in it for the sake of more consistent code for calling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds acceptable 😉

:level: :error
:audience: global
- :name: cloud_volume_snapshot_create_success
:message: 'Creating Snapshot %{snapshot_name} of Volume %{volume_name} completed successfully.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subject?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as on line 208.

Petr Blaho added 3 commits October 23, 2017 17:54
@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2017

Checked commits petrblaho/manageiq@d2ba1b0~...86e3b34 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Seal of Approval

@blomquisg
Copy link
Member

Well, if @skateman and the sea lion approve!

@blomquisg blomquisg merged commit d4c156b into ManageIQ:master Oct 23, 2017
@blomquisg blomquisg added this to the Roadmap milestone Oct 23, 2017
@skateman
Copy link
Member

@blomquisg he's my "seal of approval" 😉

@mansam
Copy link
Contributor

mansam commented Aug 10, 2018

@miq-bot add_label fine/yes

@mansam
Copy link
Contributor

mansam commented Aug 10, 2018

Marked fine/yes. Needs to be backported for https://bugzilla.redhat.com/show_bug.cgi?id=1524356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants