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

MiqAction#action_evm_event should raise MiqEvent instead of EmsEvent. #7041

Merged
merged 5 commits into from Mar 23, 2016

Conversation

lfu
Copy link
Member

@lfu lfu commented Mar 1, 2016

Usually EmsEvent is used for provider events and MiqEvent is used for MIQ events.

Alert can be set based on Datastores. But EmsEvent has fields for vm, host and ems_cluster but not for datastore.

https://bugzilla.redhat.com/show_bug.cgi?id=1306308

@lfu
Copy link
Member Author

lfu commented Mar 1, 2016

@gmcculloug @gtanzillo Please review.
If this approach is correct, other RSS feeds for Alert also need to be changed to be based on MiqEvent.
A DB migration may also be necessary to convert those EmsEvents for alert to MiqEvents.

@gtanzillo
Copy link
Member

@lfu This change definitely makes sense to me. Seems like it was misaligned on EmsEvent all of this time. Do you know if there are other places in the code where there's an expectation that events generated from this action are of type EmsEvent? Maybe timelines?

@lfu lfu changed the title MiqAction#action_evm_event should raise MiqEvent instead of EmsEvent. [WIP] MiqAction#action_evm_event should raise MiqEvent instead of EmsEvent. Mar 4, 2016
@gmcculloug gmcculloug added the wip label Mar 4, 2016
@lfu lfu force-pushed the rss_alert_wrong_title_1306308 branch 2 times, most recently from 025818c to 8859c54 Compare March 8, 2016 17:59
@lfu lfu changed the title [WIP] MiqAction#action_evm_event should raise MiqEvent instead of EmsEvent. MiqAction#action_evm_event should raise MiqEvent instead of EmsEvent. Mar 8, 2016
@lfu
Copy link
Member Author

lfu commented Mar 8, 2016

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Mar 8, 2016
@lfu
Copy link
Member Author

lfu commented Mar 8, 2016

@gtanzillo The code that expects these events generated from this action are of type EmsEvent are those .yml files under product/alerts/rss. They have been fixed to refer to MiqEvent.

opts[:ems_cluster_id] = rec.id
opts[:ems_cluster_name] = rec.name
opts[:ems_cluster_uid] = rec.uid_ems
end
Copy link
Member

Choose a reason for hiding this comment

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

@lfu @gtanzillo Should we retain setting the existing columns as well as adding in the target? I am thinking that we are filtering on at least ems_id in a number of places, like timelines.

Copy link
Member

Choose a reason for hiding this comment

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

@gmcculloug Agree, wouldn't hurt to leave that stuff in there.

lfu added 4 commits March 15, 2016 11:34
Usually EmsEvent is used for provider events and MiqEvent is used for MIQ events.
Keep the fields in MiqEvent for EVMAlertEvent. These fields are checked by event_where_clause.

Alert can be set based on Datastores. But EmsEvent does not have a field for datastore.

https://bugzilla.redhat.com/show_bug.cgi?id=1306308
So Timeline can get both EmsEvent and MiqEvent for EVMAlertEvent.
https://bugzilla.redhat.com/show_bug.cgi?id=1306308
@lfu lfu force-pushed the rss_alert_wrong_title_1306308 branch from 8859c54 to c5d790a Compare March 15, 2016 15:35
@lfu
Copy link
Member Author

lfu commented Mar 15, 2016

@h-kataria Please help verify the changes for Timeline.

@lfu lfu force-pushed the rss_alert_wrong_title_1306308 branch from c5d790a to 1a0ca0c Compare March 15, 2016 16:06
@miq-bot
Copy link
Member

miq-bot commented Mar 15, 2016

Checked commits lfu/manageiq@f55b785~...1a0ca0c with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
17 files checked, 1 offense detected

db/migrate/20160307205816_fix_event_class_for_evm_alert_event.rb

@h-kataria
Copy link
Contributor

@gmcculloug UI changes look good, timelines in UI seem to be working fine.

@chessbyte
Copy link
Member

@gmcculloug @gtanzillo please review

@gtanzillo
Copy link
Member

👍 Looks good!

@chessbyte chessbyte merged commit d9b4c6b into ManageIQ:master Mar 23, 2016
@chessbyte chessbyte added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 23, 2016
@lfu lfu deleted the rss_alert_wrong_title_1306308 branch July 14, 2016 15:11
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

6 participants