-
Notifications
You must be signed in to change notification settings - Fork 897
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 severity to alert definitions #16040
Conversation
97486b0
to
32c0aa4
Compare
32c0aa4
to
56477c3
Compare
@@ -0,0 +1,5 @@ | |||
class AddSeverityToMiqAlerts < ActiveRecord::Migration[5.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be added to manageiq-schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks @bdunne. My first migration after the split. I'll open a PR there.
a118662
to
d7b2f37
Compare
app/models/miq_alert.rb
Outdated
@@ -1,6 +1,13 @@ | |||
class MiqAlert < ApplicationRecord | |||
include UuidMixin | |||
|
|||
AVAILABLE_SEVERITIES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the word AVAILABLE signify here and in the method below? Is it possible to havemore severities? Might be better to just remove the word entirely
app/models/miq_alert.rb
Outdated
nil => nil, | ||
:info => "Information", | ||
:warning => "Warning", | ||
:error => "Error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a presentation string, or is this the value store in the database? If the former, I would expect the lookup to be the UI's domain, and if the latter, I would expect it to be downcased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fryguy Yes, that was the intention. I worked that out with @dclarizio. Dan, can this be move over to the UI side?
@Fryguy Our intentions were that the UI would simply present the options and not have to worry if they changed or some were added at a later time. Once separated, we'll have to make sure the model has it's values and the UI has the presentation strings and keep the 2 in sync. That said, I have no problem separating this out. @martinpovolny Can you help out with where we should move the presentation side of this? Do we have a decorator already perhaps that can do a dictionary lookup to get the friendly names for the drop down? |
How will these SEVERITIES be represented in the API? As strings corresponding to the symbols Or as "nice" strings? "Information", "Warning", "Error"? Who will do i18n on these? I guess the UI, right? I guess there's no other way than to have list of these on the UI side once we are purely javascript. And yes, it will need to be kept in sync with the backend. |
- Add column to miq_alerts - Allow "info", "warning", "error" or nil for no severity - Assign severity when alert fires - Allow severity in incoming event to override serenity in alert definition https://www.pivotaltracker.com/story/show/151341252
d7b2f37
to
80a6aa1
Compare
Checked commit gtanzillo@80a6aa1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/miq_alert.rb
|
Migration in ManageIQ/manageiq-schema#77
https://www.pivotaltracker.com/story/show/151341252