Conditionally update alert repeat times #2714
Conversation
|
@ZitaNemeckova @mzazrivec please review |
|
@ZitaNemeckova @mzazrivec bump |
|
@moolitayer Is it possible to call |
|
@ZitaNemeckova that can work by only resetting notification frequency for prometheus alerts and hourly alerts. That would mean that if someone has an hourly alert and then changes it to be based on a different event they would still have a 60 min notification frequency - I can live with that. Does it make sense? cc @abonas |
|
@moolitayer : are we going to merge this one or drop it? Thx! |
moolitayer
force-pushed
the
fix_prometheus_alerts
branch
from
2943afa
to
d710d81
Compare
moolitayer
changed the title
moolitayer
force-pushed
the
fix_prometheus_alerts
branch
from
d710d81
to
eceb42d
Compare
|
@martinpovolny @ZitaNemeckova ready, trying to keep #2601 while not removing behavior we want to keep |
|
ping @martinpovolny |
| @@ -368,6 +369,12 @@ def alert_default_repeat_time | |||
| end | |||
| end | |||
|
|
|||
| def apply_default_repeat_time? | |||
| (@edit[:new][:expression][:eval_method] && @edit[:new][:expression][:eval_method] == "hourly_performance") || | |||
There was a problem hiding this comment.
can you write it to using early return instead, e.g. something like:
def method
event = @edit[:new]
return true if event[:exp_event] == '_hourly_timer_'
return false if event[:eval_method].nil?
event[:eval_method] == /hourly_performance|dwh_generic/
endAlso - missing tests.
moolitayer
force-pushed
the
fix_prometheus_alerts
branch
2 times, most recently
from
52c62ca
to
55cb256
Compare
commit ea2f7ec[1] removed setting of 'notification frequency' after changing what to evaluate. Prometheus alerts relayed on that behavior and it has become impossible to create them. alert_default_repeat_time sets the value to zero and the field. is then hidden[2]. Since there is a model validation for the zero value the alerts can't be created. A similar behavior for "hourly alerts" changed - their default notification frequency should change to zero. [1] ManageIQ#2601 [2] https://github.com/ManageIQ/manageiq-ui-classic/blob/983bd1d690cd684d8e0f730f2e170fc2e3d69c7f/app/views/miq_policy/_alert_details.html.haml#L131
moolitayer
force-pushed
the
fix_prometheus_alerts
branch
from
55cb256
to
17a477f
Compare
|
Checked commit moolitayer@17a477f with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
I was getting different Unrelated |
|
@ohadlevy updated, please take a look |
|
@ohadlevy PTAL |
|
@ohadlevy @martinpovolny ping |
|
visual code inspection seems OK, thanks @moolitayer |
|
@miq-bot assign @martinpovolny |
commit ea2f7ec[1] removed setting of
'notification frequency' after changing what to evaluate. Prometheus
alerts relayed on that behavior and it has become impossible to create
them. alert_default_repeat_time sets the value to zero and the field
is then hidden[2]. Since there is a model validation for the zero value
the alerts can't be created. A similar behavior for "hourly alerts"
changed - their default notification frequency should change to 1 Hour.
[1] #2601
[2] :
manageiq-ui-classic/app/views/miq_policy/_alert_details.html.haml
Line 114 in 86b535f