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

Yamls to support backend-initiated notifications in v2v for Successful and Failed Requests #429

Conversation

AparnaKarve
Copy link

@AparnaKarve AparnaKarve commented Sep 21, 2018

@AparnaKarve AparnaKarve force-pushed the yamls_for_request_completion_notifications branch from 0407555 to 6b701f4 Compare September 21, 2018 20:25
@miq-bot
Copy link
Member

miq-bot commented Sep 21, 2018

Checked commit AparnaKarve@6b701f4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

description:
fields:
- rel5:
value: "/System/Policy/request_completed_with_failure"
Copy link
Contributor

Choose a reason for hiding this comment

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

@AparnaKarve
/System/Policy/request_completed_with_failure is not defined in the database in https://github.com/ManageIQ/manageiq-content/tree/master/content/automate/ManageIQ/System/Policy.class are you planning on using the missing instance for this

Copy link
Author

Choose a reason for hiding this comment

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

@mkanoor Should I also define request_completed_with_failure under https://github.com/ManageIQ/manageiq-content/tree/master/content/automate/ManageIQ/System/Policy.class ?
The way I have it right now seems to work.

description:
fields:
- rel5:
value: "/System/Policy/request_completed_with_success"
Copy link
Contributor

Choose a reason for hiding this comment

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

@AparnaKarve
/System/Policy/request_completed_with_success is not defined in https://github.com/ManageIQ/manageiq-content/tree/master/content/automate/ManageIQ/System/Policy.class are you planning on using the missing instance?

Copy link
Author

Choose a reason for hiding this comment

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

same question as above #429 (comment)

@mkanoor
Copy link
Contributor

mkanoor commented Sep 27, 2018

@lfu do you have comments on this?

@lfu
Copy link
Member

lfu commented Sep 27, 2018

This does not look right to me. Usually the request comes in to kick out automate actions defined in /System/Policy. But these new requests don't have any actions defined in /System/Policy.

Why these new request instances are needed for notification?
cc @gmcculloug

@AparnaKarve
Copy link
Author

@lfu We need backend triggered notifications for ServiceTemplateTransformationPlanRequest requests that have either failed or completed successfully in the v2v UI

Does it look like I will need request_completed_with_failure and request_completed_with_success in content/automate/ManageIQ/System/Policy.class, in addition to what I have?

I am not familiar with the normal practices implemented in order to support this requirement in Content/Automate, but the current approach does seem to work.

@lfu
Copy link
Member

lfu commented Sep 27, 2018

@tinaafitz @billfitzgerald0120 How do we implement notification from backend?

@AparnaKarve
Copy link
Author

Is there a prior example in the existing codebase that comes close to my current requirement that I can refer to and follow along?

@tinaafitz
Copy link
Member

Hi @AparnaKarve @lfu You can create notifications without using Automate as is shown here:
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_server.rb#L373

Can you use that approach, or do you need to have it setup in Automate content?
Automate uses some predefined generic notification types. It seems like your notifications are specific, and should be added to the notification_types table.

Can we get together on Monday to help us better understand the requirements?
/cc @mkanoor

@AparnaKarve
Copy link
Author

@tinaafitzNotification.create was exactly what I tried before I came down this route.
I have no idea why, but it does not seem to work for MiqRequest

Then I started looking up some of the existing MiqRequest notifications that do work, like request_approved (shown below)
screen shot 2018-09-28 at 10 55 27 am

What I noticed was, request_approved and request_denied type of notifications were using call_automate_event_queue
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L254
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L270

which is what I have used in ManageIQ/manageiq#18012
(this is the supporting MIQ core PR for the current PR)

I believe my use case is very similar to the above two (request_approved and request_denied), except that I'm targeting MiqRequests of the type ServiceTemplateTransformationPlanRequest, which are v2v specific

Let's get together on Monday and we can discuss this more.
Thanks Tina!

@AparnaKarve
Copy link
Author

As mentioned above call_automate_event_queue seems like the only way to go to create Notifications from MiqRequest, @tinaafitz @lfu let me know if you have other suggestions.

@tinaafitz
Copy link
Member

tinaafitz commented Oct 1, 2018

@AparnaKarve I'm wondering if it's possible that the Notification.create didn't work properly initially because there wasn't a notification type defined for your new notification.
Have you tried calling it with a pre-existing notification type?

@AparnaKarve
Copy link
Author

@tinaafitz That could very well be the case that the new Notification type was somehow not found when the Notification.create call was made.

This is something really strange -- even though the new Notification type exists in the notification_types table, the Notification.create call fails from the app and also from rails console. Any idea why that could be?

@AparnaKarve
Copy link
Author

AparnaKarve commented Oct 2, 2018

@tinaafitz I may have identified the issue with Notification.create ...

It looks like in notification_types.yml, when I set the audience to either user or group, Notification.create does not seem to work.

In Rails console, I get the following error if I use a notification_type that has it's audience set to user/group

> Notification.create(:type => "generic_task_finish", :options => {:message => "test"})
  NotificationType Load (0.5ms)  SELECT  "notification_types".* FROM "notification_types" WHERE "notification_types"."name" = $1 LIMIT $2  [["name", "generic_task_finish"], ["LIMIT", 1]]
  NotificationType Inst Including Associations (0.1ms - 1rows)
   (0.2ms)  BEGIN
   (0.1ms)  ROLLBACK
NoMethodError: undefined method `id' for nil:NilClass
	from ~/manageiq/app/models/notification_type.rb:22:in `subscriber_ids'
	from ~/manageiq/app/models/notification.rb:65:in `set_notification_recipients'
	from /Users/akarve/.rvm/gems/ruby-2.4.1/gems/activesupport-5.0.7/lib/active_support/callbacks.rb:382:in `block in make_lambda'
	from /Users/akarve/.rvm/gems/ruby-2.4.1/gems/activesupport-5.0.7/lib/active_support/callbacks.rb:169:in `block (2 levels) in halting'

So I have now changed the audience of my new notification_types to global , which seems to work.

:audience: global

With the changes made in ManageIQ/manageiq#18012, it looks like we no longer need this PR.

Thank you for your tips -- really appreciated!!

EDIT: If audience is set to user, it looks like we need to pass :user_id => user_id to Notification.create
Anyway, the global audience seems to be OK for my particular case.

@AparnaKarve AparnaKarve closed this Oct 2, 2018
@AparnaKarve AparnaKarve deleted the yamls_for_request_completion_notifications branch October 2, 2018 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants