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

Pass additional metadata from alert to event #14301

Merged
merged 1 commit into from
Mar 29, 2017

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented Mar 13, 2017

Depends on #14295 [merged]

@moolitayer
Copy link
Author

@miq-bot add_label enhancement, providers/containers, wip

@moolitayer moolitayer force-pushed the additional_alert_meta branch 2 times, most recently from 46bb8ff to 916ddff Compare March 17, 2017 12:55
@moolitayer moolitayer changed the title [WIP] Pass additional metadata from alert to event Pass additional metadata from alert to event Mar 17, 2017
@moolitayer
Copy link
Author

@miq-bot remove_label wip
(dependency merged)

@miq-bot miq-bot removed the wip label Mar 17, 2017
@moolitayer
Copy link
Author

@gmcculloug can you please review?

@moolitayer
Copy link
Author

@simon3z @gmcculloug could you please review?

@@ -178,6 +178,8 @@ def parse_event_metadata
event_type == "datawarehouse_alert" ? message : nil,
full_data.try(:[], :severity),
full_data.try(:[], :url),
full_data.try(:[], :ems_ref),
full_data.try(:[], :resolved),
Copy link
Member

Choose a reason for hiding this comment

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

The try logic is getting ugly. It makes more sense to initialize full_data at the top of the method if it is nil then just reference the hash normally.

def parse_event_metadata
  full_data ||= {}
  [
    event_type == "datawarehouse_alert" ? message : nil,
    full_data.[:severity],
    full_data[:url],
    full_data[:ems_ref],
    full_data[:resolved]
  ]
end

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commit moolitayer@95ebfe0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

@moolitayer
Copy link
Author

moolitayer commented Mar 21, 2017

@gmcculloug could you take another look please?

@simon3z
Copy link
Contributor

simon3z commented Mar 21, 2017

Code-wise LGTM 👍
Anyway this is more about the implications on the core infrastructure. I'll leave to @gmcculloug to evaluate that (e.g. is this playing nicely with all providers, etc.)

@miq-bot assign gmcculloug

@miq-bot miq-bot assigned gmcculloug and unassigned simon3z Mar 21, 2017
@moolitayer
Copy link
Author

@gmcculloug please take another look

@gmcculloug gmcculloug merged commit 0503155 into ManageIQ:master Mar 29, 2017
@gmcculloug gmcculloug added this to the Sprint 58 Ending Apr 10, 2017 milestone Mar 29, 2017
@gmcculloug
Copy link
Member

@moolitayer Please label fine/yes or fine/no

@moolitayer
Copy link
Author

@miq-bot add_label fine/no

@miq-bot miq-bot added the fine/no label Apr 2, 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.

4 participants