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

Add description to miq_alert_statuses #13653

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented Jan 25, 2017

Why cookie description from miq_alert to miq_alert_statuses?
We want to support generic alerting from external systems (hawkular).
What we get from it is only instances of an alert (==miq_alert_status) containing a description and there is no entity parallel to miq_alert.

The purpose of this change is to take the description from the MAS when it is available (container alerts from hawkular) or from the MA otherwise (all other providers including middleware alerts)

The piece adding the external description on mas will come in the event collection patch #12773

@moolitayer
Copy link
Author

cc @simon3z @jeff-phillips-18 This is called "message" in the mockups

@moolitayer
Copy link
Author

@miq-bot add_label enhancement, wip

@moolitayer moolitayer force-pushed the add_status_description branch 2 times, most recently from b0688ec to 2fd3d26 Compare January 27, 2017 21:38
@moolitayer moolitayer changed the title [WIP] Add description to miq_alert_statuses Add description to miq_alert_statuses Jan 27, 2017
@moolitayer moolitayer force-pushed the add_status_description branch from 2fd3d26 to a07ea2d Compare January 27, 2017 22:37
@moolitayer
Copy link
Author

@miq-bot remove_label wip

@kbrock can you review the migration(+spec) ?
@simon3z Please review

@miq-bot miq-bot removed the wip label Jan 27, 2017

result
end

def add_status_post_evaluate(target, result)
def add_status_post_evaluate(target, result, statue_description)
Copy link
Contributor

Choose a reason for hiding this comment

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

@moolitayer typo: statue_description

@moolitayer moolitayer force-pushed the add_status_description branch from a07ea2d to 18453d7 Compare January 30, 2017 15:29
@miq-bot
Copy link
Member

miq-bot commented Jan 30, 2017

Checked commit moolitayer@18453d7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 1 offense detected

db/migrate/20170125141953_update_description_in_miq_alert_status.rb

miq_alert_statuses = Arel::Table.new('miq_alert_statuses')
join_sql = miq_alerts.project(miq_alerts[:description])
.where(miq_alerts[:id].eq(miq_alert_statuses[:miq_alert_id])).to_sql
MiqAlertStatus.update_all("description = (#{join_sql})")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this one. We need someone to properly review it. Maybe @kbrock

Copy link
Member

Choose a reason for hiding this comment

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

Heh, I would have done sql instead of areal but it looks good.

update miq_alert_statuses
set description = (
  select miq_alerts.description
  from miq_alerts
  where miq_alerts.id = miq_alert_statuses.miq_alert_id
)

LGTM 👍

@simon3z
Copy link
Contributor

simon3z commented Jan 30, 2017

LGTM 👍

@gtanzillo can you check if this is OK for Alerts in general?

@miq-bot assign gtanzillo

@miq-bot miq-bot assigned gtanzillo and unassigned simon3z Jan 30, 2017
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 @Fryguy, please review for SQL migrations.

@Fryguy Fryguy merged commit 6e41ec1 into ManageIQ:master Feb 1, 2017
@Fryguy Fryguy added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 1, 2017
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.

6 participants