Add alerts drop down to provider view #1918

nimrodshn · 2017-08-15T13:42:01Z

Adding alert drop down to container provider view.
GIF:

Screenshot of the produced endpoint:

nimrodshn · 2017-08-15T13:48:28Z

@moolitayer This PR is depending on #1501 where we change retrieve_metrics_selection at container_controller, currently to retrieve the metrics we check @ems.endpoints.count == 1 ? 'hawkular_disabled' : 'hawkular_enabled' this of course will be false in the case of added prometheus_alerts which will in turn make hawkular tab enabled.

nimrodshn · 2017-08-21T11:18:00Z

@moolitayer PTAL (ignore code climate 'similar code' issues, according to @himdel it is set too high..)

moolitayer · 2017-08-21T11:39:39Z

@nimrodshn how are we doing validation for this endpoint? we should use the code we have in monitoring_manager.verify_credentials (we might need to expose it differently)

nimrodshn · 2017-08-21T12:21:59Z

@moolitayer we do use monitoring_manager.verify_credentials.

You can look here: authentication_check (which is the method used when the client clicks on validate) calls authentication_check_no_validation which calls verify_credentials.

moolitayer · 2017-08-23T15:32:18Z

@nimrodshn it's not using monitoring_manager.verify_credentials. It's using container_manager_mixin.verify_credentials. A change is needed in the backend and i'm not really sure how to handle that, anyway I will handle that.

nimrodshn · 2017-08-23T16:58:17Z

@moolitayer good catch! for some reason the trace in my IDE led me to believe we are using monitoring manager.. let me know if there is something I can do to help with that.

ilackarms · 2017-08-27T14:21:00Z

alerts endpoint configuration isn't producing an Authentication

from rails c:

irb(main):014:0> ap ems.connection_configurations[:prometheus_alerts]
OpenStruct {
          :endpoint => #<Endpoint:0x0056215892a9a8> {
                           :id => 3,
                         :role => "prometheus_alerts",
                    :ipaddress => nil,
                     :hostname => "alerts-kube-system.10.35.48.200.nip.io",
                         :port => 443,
                :resource_type => "ExtManagementSystem",
                  :resource_id => 1,
                   :created_at => Sun, 27 Aug 2017 12:40:45 UTC +00:00,
                   :updated_at => Sun, 27 Aug 2017 14:07:54 UTC +00:00,
                   :verify_ssl => 0,
                          :url => nil,
            :security_protocol => "ssl-without-validation",
                  :api_version => nil,
                         :path => nil,
        :certificate_authority => nil
    },
    :authentication => nil
}
=> nil

nimrodshn · 2017-08-27T14:40:32Z

@ilackarms good catch! @moolitayer could this be related to our previous discussion?

moolitayer · 2017-08-27T14:42:11Z

@nimrodshn This is something else.

Looks like you are creating the endpoint correct but missing the authentication.
You will need to add the new authentication, I think it starts here[1]
this is what you should have eventually[2]
Pay special attention to:

:role => "prometheus_alerts",
:authtype => "prometheus_alerts",

[1] https://github.com/nimrodshn/manageiq-ui-classic/blame/2a723c03de96e3d5c6e9365a27216bbf597463e2/app/controllers/mixins/ems_common_angular.rb#L529
[2] https://paste.fedoraproject.org/paste/L9J4KLFRFiYQygOtlchekw

moolitayer · 2017-08-27T14:45:16Z

@nimrodshn btw if you run the alerts collection you can make sure the new endpoint is configured correctly.

moolitayer · 2017-08-27T14:46:59Z

@nimrodshn can you remove the WIP please?
We have the Monitoring Manager in master for some time now.

nimrodshn · 2017-08-28T07:57:53Z

@moolitayer just added authentication.
Screenshot:

miq-bot · 2017-08-30T10:34:02Z

This pull request is not mergeable. Please rebase and repush.

moolitayer · 2017-09-06T14:01:10Z

@dclarizio please review

yaacov · 2017-09-06T14:04:43Z

@moolitayer @ilackarms can you report here that this is tested and working on your environments ?
I think that will help the UI guys reiew this.

moolitayer · 2017-09-06T14:07:50Z

Right, tested today and working as expected!

AparnaKarve · 2017-09-06T17:50:36Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

@@ -247,10 +264,15 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
      ($scope.emsCommonModel.default_hostname != '' && $scope.emsCommonModel.default_api_port) &&
      ($scope.emsCommonModel.default_password != '' && $scope.angularForm.default_password.$valid)) {
      return true;
-    } else if(($scope.emsCommonModel.metrics_selection != "disabled" && $scope.emsCommonModel.ems_controller == "ems_container") &&
+    } else if(($scope.emsCommonModel.ems_controller == "ems_container") &&


Use === instead of ==

Also, a space is preferred after if per style guide

@AparnaKarve Thanks 👍 the if with no space, looked wrong ... but codeclimate didn't shout so I thought is was ok :-)

AparnaKarve · 2017-09-06T17:53:38Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

      ($scope.emsCommonModel.emstype) &&
-      ($scope.emsCommonModel.metrics_hostname != '' && $scope.emsCommonModel.metrics_api_port) &&
-      ($scope.emsCommonModel.default_password != '' && $scope.angularForm.default_password.$valid)) {
+      ($scope.emsCommonModel.default_password != '' && $scope.angularForm.default_password.$valid) &&


Similarly !== instead of !=

Please apply this elsewhere too (to all your PR changes where this is applicable)

AparnaKarve · 2017-09-06T17:56:48Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

+        amqp: {},
+        metrics: {},
+        ssh_keypair: {},
+        prometheus_alerts: {}


trailing comma is missing

👍 Thanks ! how did this pass code climate ?

AparnaKarve · 2017-09-06T18:02:29Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

    } else {
-      angular.element("#metrics_tab").show();
+      angular.element(tabSelector).show();
    }


This code is OK for the first pass, since it has already been tested.
But I'm wondering if we should move this code in a directive later.

Yes 👍 , the whole ruby = select_tag(... in the .haml file, should be in angular so will not need this hack :-(

AparnaKarve · 2017-09-06T18:09:51Z

Based on what I saw in the uploaded video above, when the Alerts tab is added, it contains fields inside it that are Required.
If that is true, the Alerts tab should have an error indicator (red exclaimation mark) on it.

It is essential to have the error indicator on the tab to be consistent with the rest of our UI and also for better UX.

If you want to implement this in a follow-up PR, that would be perfectly OK.

yaacov · 2017-09-06T18:15:27Z

If you want to implement this in a follow-up PR, that would be perfectly OK.

We do (*)

(*) Actually we are currently fixing the validation code in the backend :-( and we may need to retouch this UI if we find that we missed something that we didn't see because of a bug in the backend (like what happened in #2102 )

yaacov · 2017-09-06T18:22:25Z

Based on what I saw in the uploaded video above

@ilackarms @moolitayer the screenshots here are obsolete, do you have current screenshots of what this PR looks now ?

cc @AparnaKarve

AparnaKarve · 2017-09-06T18:32:07Z

app/views/layouts/angular/_multi_auth_credentials.html.haml

@@ -24,7 +24,7 @@
          = _("Metrics")
      = miq_tab_header('alerts', nil, 'ng-click' => "changeAuthTab('alerts')") do
        %div{"ng-if" => "emsCommonModel.alerts_selection == 'prometheus'"}
-          %i{"error-on-tab" => "alerts", :style => "color:#cc0000"}
+          %i{"error-on-tab" => "prometheus_alerts", :style => "color:#cc0000"}


Nice! Easy enough! :)

yaacov · 2017-09-06T18:34:04Z

@AparnaKarve 👍 Thanks, I think the tab missing "error" sign is fixed, see last commit.

A screen shot of none valid alerts tab:

miq-bot · 2017-09-06T18:34:11Z

Checked commits nimrodshn/manageiq-ui-classic@a67c6ed~...6da318b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

AparnaKarve

@yaacov Thanks for the updated screenshot and all the other code updates.

@dclarizio LGTM and GTG

nimrodshn changed the title ~~add alerts drop down to provider view~~ [WIP] Add alerts drop down to provider view Aug 15, 2017

miq-bot added the wip label Aug 15, 2017

nimrodshn force-pushed the add_alert_drop_down branch 4 times, most recently from c2d3384 to 2a723c0 Compare August 21, 2017 09:46

nimrodshn changed the title ~~[WIP] Add alerts drop down to provider view~~ Add alerts drop down to provider view Aug 28, 2017

miq-bot removed the wip label Aug 28, 2017

nimrodshn force-pushed the add_alert_drop_down branch 2 times, most recently from d263025 to fdc8e27 Compare August 29, 2017 05:44

miq-bot added the unmergeable label Aug 30, 2017

yaacov force-pushed the add_alert_drop_down branch 2 times, most recently from fdc8e27 to 4a35d46 Compare August 31, 2017 10:50

miq-bot removed the unmergeable label Aug 31, 2017

yaacov force-pushed the add_alert_drop_down branch 3 times, most recently from 59cfd30 to c09bedd Compare August 31, 2017 12:19

yaacov added 3 commits September 6, 2017 15:03

fix merge conflicts

6689316

remove goast tab when disabled

73060b3

remove alert/metrics dup

ac68b9d

yaacov force-pushed the add_alert_drop_down branch from f620892 to 5026e76 Compare September 6, 2017 12:06

miq-bot removed the unmergeable label Sep 6, 2017

yaacov force-pushed the add_alert_drop_down branch from 5026e76 to 5196725 Compare September 6, 2017 12:09

add the endpoint and authentication for alerts

3228bb5

yaacov force-pushed the add_alert_drop_down branch from 5196725 to 3228bb5 Compare September 6, 2017 12:18

dclarizio added the fine/no label Sep 6, 2017

dclarizio requested a review from AparnaKarve September 6, 2017 15:38

dclarizio self-assigned this Sep 6, 2017

AparnaKarve reviewed Sep 6, 2017

View reviewed changes

fix style issues

473c334

yaacov force-pushed the add_alert_drop_down branch from 2872702 to 473c334 Compare September 6, 2017 18:09

fix missing error sign in the alerts tab

6da318b

AparnaKarve reviewed Sep 6, 2017

View reviewed changes

AparnaKarve approved these changes Sep 6, 2017

View reviewed changes

dclarizio merged commit 660dce0 into ManageIQ:master Sep 6, 2017

dclarizio added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alerts drop down to provider view #1918

Add alerts drop down to provider view #1918

nimrodshn commented Aug 15, 2017

nimrodshn commented Aug 15, 2017 •

edited

Loading

nimrodshn commented Aug 21, 2017 •

edited

Loading

moolitayer commented Aug 21, 2017

nimrodshn commented Aug 21, 2017 •

edited

Loading

moolitayer commented Aug 23, 2017

nimrodshn commented Aug 23, 2017 •

edited

Loading

ilackarms commented Aug 27, 2017

nimrodshn commented Aug 27, 2017

moolitayer commented Aug 27, 2017

moolitayer commented Aug 27, 2017

moolitayer commented Aug 27, 2017

nimrodshn commented Aug 28, 2017

miq-bot commented Aug 30, 2017

moolitayer commented Sep 6, 2017

yaacov commented Sep 6, 2017

moolitayer commented Sep 6, 2017

AparnaKarve Sep 6, 2017

yaacov Sep 6, 2017

AparnaKarve Sep 6, 2017

AparnaKarve Sep 6, 2017

yaacov Sep 6, 2017

AparnaKarve Sep 6, 2017

yaacov Sep 6, 2017

AparnaKarve commented Sep 6, 2017

yaacov commented Sep 6, 2017

yaacov commented Sep 6, 2017

AparnaKarve Sep 6, 2017

yaacov Sep 6, 2017

yaacov commented Sep 6, 2017

miq-bot commented Sep 6, 2017

AparnaKarve left a comment

Add alerts drop down to provider view #1918

Add alerts drop down to provider view #1918

Conversation

nimrodshn commented Aug 15, 2017

nimrodshn commented Aug 15, 2017 • edited Loading

nimrodshn commented Aug 21, 2017 • edited Loading

moolitayer commented Aug 21, 2017

nimrodshn commented Aug 21, 2017 • edited Loading

moolitayer commented Aug 23, 2017

nimrodshn commented Aug 23, 2017 • edited Loading

ilackarms commented Aug 27, 2017

nimrodshn commented Aug 27, 2017

moolitayer commented Aug 27, 2017

moolitayer commented Aug 27, 2017

moolitayer commented Aug 27, 2017

nimrodshn commented Aug 28, 2017

miq-bot commented Aug 30, 2017

moolitayer commented Sep 6, 2017

yaacov commented Sep 6, 2017

moolitayer commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Sep 6, 2017

yaacov commented Sep 6, 2017

yaacov commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Sep 6, 2017

miq-bot commented Sep 6, 2017

AparnaKarve left a comment

Choose a reason for hiding this comment

nimrodshn commented Aug 15, 2017 •

edited

Loading

nimrodshn commented Aug 21, 2017 •

edited

Loading

nimrodshn commented Aug 21, 2017 •

edited

Loading

nimrodshn commented Aug 23, 2017 •

edited

Loading