Prometheus Alerts Endpoint: add detection #2663
Conversation
|
@miq-bot add_label gaprindashvili/yes, bug |
moolitayer
force-pushed
the
alerts_detection
branch
3 times, most recently
from
d6483cb
to
4882abf
Compare
|
@yaacov @AparnaKarve please review. |
| $scope.updateAlertsHostname(data.hostname); | ||
| } else { | ||
| $scope.updateMetricsHostname(data.hostname); | ||
| } |
There was a problem hiding this comment.
Can these 2 functions be consolidated into 1?
$scope.updateHostname(data.hostname);$scope.updateHostname = function(value) {
if ($scope.currentTab === "alerts") {
$scope.emsCommonModel.prometheus_alerts_hostname = value;
} else {
$scope.emsCommonModel.metrics_hostname = value;
}
};
| @@ -84,7 +84,8 @@ | |||
| "checkchange" => "", | |||
| "required" => "", | |||
| "selectpicker-for-select-tag" => "") | |||
|
|
|||
| %input{:type => 'text', :id => "current_tab", :name => "current_tab", "ng-model" => "currentTab", | |||
| "ng-hide" => "true"} | |||
There was a problem hiding this comment.
can you change this to the standard way of writing a hash - which is 1 key/value per line?
or include everything in 1 line.
|
@moolitayer Looks good for the most part, with just a few minor things above. |
| "hawkular" => %w(hawkular-metrics openshift-infra), | ||
| "prometheus" => %w(prometheus prometheus) | ||
| "hawkular" => %w(hawkular-metrics openshift-infra), | ||
| "prometheus" => %w(prometheus prometheus), |
There was a problem hiding this comment.
it's just got changed to: prometheus openshift-metrics :-)
|
I'm also fine with that except for @AparnaKarve things about the new project name for the prometheus route, we can do it in a new PR or here ... |
moolitayer
force-pushed
the
alerts_detection
branch
from
4882abf
to
60c5958
Compare
|
@AparnaKarve @yaacov addressed comments PTAL Applied the Prometheus project name change to the metrics as well. |
| @@ -611,12 +611,16 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', ' | |||
| }); | |||
| }; | |||
|
|
|||
| $scope.updateAuthStatus = function(updatedValue) { | |||
There was a problem hiding this comment.
don't we need the $scope.updateAuthStatus function any more ?
It is removed in a PR that does not deal with authStatus, what am I missing ?
There was a problem hiding this comment.
@moolitayer I think you removed the wrong function.
You probably meant to remove updateAlertsHostname.
@yaacov Thanks.
moolitayer
force-pushed
the
alerts_detection
branch
from
60c5958
to
e2dc118
Compare
|
Updated tests for the new hostname |
|
Checked commits moolitayer/manageiq-ui-classic@772cfa6~...e2dc118 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
|
LGTM as well. |
|
Thanks for helping out @AparnaKarve 🎉 |
Prometheus Alerts Endpoint: add detection (cherry picked from commit a458f49)
|
Gaprindashvili backport details: |
Currently for OpenShift we have detection for both metrics implementations (Hawkular, Prometheus)
but not for alerts (Prometheus).