Hawkular hostname detection changes #1304

moolitayer · 2017-05-07T15:20:22Z

In container providers, before this change, if the hostname of a Hawkular endpoint is empty upon submission - we will attempt to detect it from a route leading to it in OpenShift. There is no indication to the user on the detection and no way of seeing the detected value pre submission.

This PR moves detection to be a button that only fills the form value.

no default hostname/port to use for detection - detect button is disabled

detection available

after detection validate becomes enabled

bug: https://bugzilla.redhat.com/show_bug.cgi?id=1437138

moolitayer · 2017-05-07T15:20:31Z

@miq-bot add_label wip

moolitayer · 2017-05-08T12:14:24Z

@miq-bot remove_label wip

moolitayer · 2017-05-08T12:14:45Z

@zeari can you please review this change?

zeari · 2017-05-08T12:18:09Z

~~@moolitayer Screenshots please!~~
Thanks

zeari · 2017-05-08T12:44:35Z

@moolitayer Overall look good. Im just not sure how this handles various errors. for example if someone tries to use detect when they didnt fill out provider details correctly.

anyway this need a UI review. @dclarizio @himdel PTAL

moolitayer · 2017-05-08T13:34:52Z

@moolitayer Overall look good. Im just not sure how this handles various errors. for example if someone tries to use detect when they didnt fill out provider details correctly.

The detect button will be grayed out if hostname and or port weren't inserted

moolitayer · 2017-05-08T13:35:10Z

@cben can you review these changes?

AparnaKarve · 2017-05-08T23:52:53Z

app/controllers/mixins/ems_common_angular.rb

-      return nil unless ems.class.respond_to?(:openshift_connect)
-      endpoint = Endpoint.new(endpoint_hash)
+    # TODO: move to backend
+    def get_hostname_from_routes(ems)


@moolitayer Can you move this method and all the other container specific code in this PR to ems_container_controller?

moolitayer · 2017-05-09T09:50:55Z

@miq-bot add_label compute/containers, enhancement, fine/yes
(per bz)

dclarizio · 2017-05-09T15:18:40Z

@moolitayer can you please review the codeclimate issues, some look easy to fix? Thx, Dan

It may be just me, but when does the Detect button light up? Is it when the user has a port entered as well as a Name and type, above? Hard to tell from the screen shot. Does the disabled button have hover text to let you know what it takes to light it up?

If the detection end point times out or has errors, what is the end user experience? Are we spinning or waiting for a potentially long time?

himdel · 2017-05-10T09:01:43Z

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

+    miqService.sparkleOn();
+    miqService.detectWithRest($event, $scope.actionUrl)
+      .then(function success(data) {
+          $scope.$apply(function() {


Suddenly 4 space indent? ;)

himdel · 2017-05-10T09:02:01Z

app/assets/javascripts/services/miq_service.js

@@ -66,6 +66,12 @@ ManageIQ.angular.app.service('miqService', ['$timeout', '$document', '$q', funct
    }, 200);
  };

+  this.detectWithRest = function($event, url) {
+      angular.element('#button_name').val('detect');


himdel · 2017-05-10T09:03:28Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

@@ -77,6 +77,16 @@
                          "checkchange"             => "",
                          "prefix"                  => prefix.to_s,
                          "reset-validation-status" => "#{prefix}_auth_status"}
+      - detect = "detectClicked({target: '.detect_button'}, true)"


The indent level should be the same as that %span on the next line, otherwise, it looks like you're declaring a variable for a block that is no longer there.

himdel · 2017-05-10T09:06:44Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

@@ -77,6 +77,16 @@
                          "checkchange"             => "",
                          "prefix"                  => prefix.to_s,
                          "reset-validation-status" => "#{prefix}_auth_status"}
+      - detect = "detectClicked({target: '.detect_button'}, true)"
+    %span
+      %miq-button.detect_button{:ng_if => defined?(detection) ? true : false,


We're doing - foo ||= false for the other variables on the beginning of this partial - can you do the same for detection please? (So that you don't need that defined?).

Also.. if you're trying to use ng-if, it has to be 'ng-if', not :ng_if .. and that true and false should probably be in quotes, unless you're interested in the presence of such attribute, instead of its value.

moolitayer · 2017-05-10T17:13:09Z

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

@@ -522,6 +542,11 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
    $scope.angularForm[$scope.authType + '_auth_status'].$setViewValue(updatedValue);
  };

+  $scope.updateHawkularHostname = function(value) {
+    $scope.angularForm['hawkular_hostname'].$setViewValue(value);
+    $('#hawkular_hostname').val(value);


@himdel is there a better way for the above?

@moolitayer You could use

angular.element('#hawkular_hostname').val(value);

which is also recommended by CodeClimate.

Thanks @AparnaKarve

Uh... but.. why not simply use $scope.emsCommonModel.hawkular_hostname = value? Since hawkular_hostname is an input with ng-model, no reason to access it magically...

no reason to access it magically

Sure, if just the assignment $scope.emsCommonModel.hawkular_hostname = value works, why not?

moolitayer · 2017-05-11T13:11:33Z

@moolitayer can you please review the codeclimate issues, some look easy to fix? Thx, Dan

It may be just me, but when does the Detect button light up? Is it when the user has a port entered as well as a Name and type, above? Hard to tell from the screen shot. Does the disabled button have hover text to let you know what it takes to light it up?

If the detection end point times out or has errors, what is the end user experience? Are we spinning or waiting for a potentially long time?

The detection is using fields from the other endpoint, so the button becomes available when those fields are available. I tested error flows (detection fails) and error messages are showing. There is no timeout so the spinner will not go away. I added disabled and enabled titles.

moolitayer · 2017-05-11T13:15:03Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

+    %span
+      %miq-button.detect_button{"ng-if" => detection ? 'true' : 'false',
+                  :name            => _("Detect"),
+                  :on_click        => detect,


@himdel these attribute names seem to work.
Do you want them changed to strings with -?
(:on_click, :disabled_title, :enabled_title:)

Aaah.. according to https://docs.angularjs.org/api/ng/type/$compile.directive.Attributes , angular accepts dashes, underscores or colons .. so .. yeah, good to know.

But, we always use the dash version, so.. if you don't mind, to prevent potential confusion..

moolitayer · 2017-05-29T09:45:16Z

@himdel @cben please take a look

cben · 2017-05-29T17:26:28Z

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

+  $scope.detectClicked = function($event) {
+    miqService.sparkleOn();
+    miqService.detectWithRest($event, $scope.actionUrl)
+      .then(function success(data) {


perhaps should also have a failure callback? (to display a flash)

This callback also handles detection failure as an error is returned (tested) similarly to the validate function

as for failure to get the response there's not much we can do if we can't talk to the server - so I think it is good enough to do the same as the validate callback.

cben · 2017-05-29T17:28:55Z

app/controllers/ems_container_controller.rb

+  def get_hostname_from_routes(ems)
+    return nil, "Route detection not applicable for provider type" unless ems.class.respond_to?(:openshift_connect)
+    [
+      ems.connect(:service => :openshift).get_route('hawkular-metrics', 'openshift-infra').try(:spec).try(:host),


cben · 2017-05-29T17:36:33Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

+                  :name            => _("Detect"),
+                  "on-click"       => detect,
+                  :enabled         => "isDetectionEnabled()",
+                  "disabled-title" => _("Detection not available for provider type"),


is this the reason it would be visible but disabled?
I think for other providers you just hide it & this should explain this fields you have to fill.

cben · 2017-05-29T17:45:31Z

2 tiny comments but this has long Looked Good To Me... Let me know if you want me to try it out.

moolitayer · 2017-06-04T12:23:26Z

@himdel @cben please take a look

moolitayer · 2017-06-05T10:15:48Z

@himdel please take a look

himdel · 2017-06-06T11:37:11Z

So... the styling is a bit broken when both Detect and Required are shown..

EDIT: Required is broken regardless. In all the other fields, Required is on the new line, not to the right of the input.

himdel · 2017-06-06T11:43:28Z

This should fix the styling problem:

diff --git a/app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml b/app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml
index 170ccd58a0..a1edd3f234 100644
--- a/app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml
+++ b/app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml
@@ -79,6 +79,11 @@
                           "checkchange"             => "",
                           "prefix"                  => prefix.to_s,
                           "reset-validation-status" => "#{prefix}_auth_status"}
+      %span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.required"}
+        = _("Required")
+      %span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.detectedSpaces"}
+        = _("Spaces are prohibited")
+
     - detect = "detectClicked({target: '.detect_button'})"
     %span
       %miq-button.detect_button{"ng-if" => detection ? 'true' : 'false',
@@ -90,11 +95,6 @@
                   :xs              => 'true',
                   :primary         => 'true'}
 
-      %span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.required"}
-        = _("Required")
-      %span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.detectedSpaces"}
-        = _("Spaces are prohibited")
-
 %div{"ng-if" => defined?(apiport_hide) ? false : true}
   .form-group{"ng-class" => "{'has-error': angularForm.#{prefix}_api_port.$invalid}",
               "ng-if" => "emsCommonModel.emstype == 'openstack'              || " + |

himdel · 2017-06-06T11:49:51Z

app/assets/javascripts/services/miq_service.js

+  this.detectWithRest = function($event, url) {
+    angular.element('#button_name').val('detect');
+    miqSparkleOn();
+    return miqRESTAjaxButton(url, $event.target, 'json');


When wrapping a function returning a non-angular Promise, it's a good idea to wrap the result in an angular promise - so that you don't need that $scope.$apply when calling it.

return $q.when(miqRESTAjaxButton(url, $event.target, 'json')); should work ;)

himdel · 2017-06-06T11:50:44Z

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

+    miqService.sparkleOn();
+    miqService.detectWithRest($event, $scope.actionUrl)
+      .then(function success(data) {
+        $scope.$apply(function() {


..And you can remove this $scope.$apply :) (https://github.com/ManageIQ/manageiq-ui-classic/pull/1304/files#r120332564)

himdel · 2017-06-06T11:54:21Z

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

+                  "enabled-title"  => _("Detect Endpoint"),
+                  :xs              => 'true',
+                  :primary         => 'true'}
+
      %span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.required"}


(these error %spans need to be siblings of the %input, not cousins) (#1304 (comment))

moolitayer · 2017-06-07T15:29:42Z

Thanks for the catch @himdel, seems fine now:

kindly rereview

miq-bot · 2017-06-07T15:33:50Z

Checked commit moolitayer@9c30024 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

himdel · 2017-06-07T16:01:33Z

@moolitayer Thanks, LGTM, just one little detail I notice.. this does not work for Kubernetes, only OpenShift... so you can look at the detect button, see the "Required information missing" label, and wonder what else it needs :).

Maybe make that disabled-title something like "Required information missing; currently only supported for Openshift"? Or, maybe better, ng-if="emsCommonModel.emstype == 'openshift'"?

moolitayer · 2017-06-07T16:26:34Z

Detection isn't available if a required filed is missing OR the provider type is kubernetes.
I first had the second option in the error message but changed it after a review comment.

I could allow the detection for kubernetes and return a message from the server if that is a good solution.

Note that in a real life scenario someone using kubernetes would disable hawkular since it is not available there (hmm... we should probably disable hawkular when someone selects kubernetes )

himdel · 2017-06-07T16:30:33Z

Aaah.. hawkular is not used with kubernetes at all?

If so, then it makes sense to just merge this and remove that hawkular code in a separate PR, right?

moolitayer · 2017-06-07T16:35:51Z

yes to remove the hawkular option when kubernetes is selected

moolitayer · 2017-06-13T14:00:41Z

@miq-bot add_label fine/no

miq-bot added the wip label May 7, 2017

moolitayer force-pushed the detect_hawkular branch from b66619d to 734fe6f Compare May 8, 2017 12:14

moolitayer changed the title ~~[WIP] Hawkular hostname detection changes~~ Hawkular hostname detection changes May 8, 2017

miq-bot removed the wip label May 8, 2017

AparnaKarve reviewed May 8, 2017

View reviewed changes

miq-bot added compute/containers enhancement fine/yes labels May 9, 2017

moolitayer force-pushed the detect_hawkular branch 2 times, most recently from c0a667a to 549a24f Compare May 9, 2017 10:47

dclarizio assigned himdel May 9, 2017

himdel reviewed May 10, 2017

View reviewed changes

moolitayer force-pushed the detect_hawkular branch 2 times, most recently from f1b0f9f to 10f005a Compare May 10, 2017 17:11

moolitayer commented May 10, 2017

View reviewed changes

moolitayer force-pushed the detect_hawkular branch from 10f005a to 82ba9c4 Compare May 11, 2017 12:56

moolitayer commented May 11, 2017

View reviewed changes

moolitayer closed this May 29, 2017

moolitayer reopened this May 29, 2017

cben reviewed May 29, 2017

View reviewed changes

moolitayer force-pushed the detect_hawkular branch from 9bff73f to def03e5 Compare June 1, 2017 11:47

cben approved these changes Jun 5, 2017

View reviewed changes

himdel reviewed Jun 6, 2017

View reviewed changes

moolitayer force-pushed the detect_hawkular branch from def03e5 to 9b8375f Compare June 7, 2017 15:28

Hawkular hostname detection changes

9c30024

moolitayer force-pushed the detect_hawkular branch from 9b8375f to 9c30024 Compare June 7, 2017 15:31

himdel merged commit 93aa552 into ManageIQ:master Jun 7, 2017

himdel added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 7, 2017

himdel mentioned this pull request Jun 8, 2017

Remove "Confirm Password" input field previously required for Validation #1335

Merged

miq-bot added the fine/no label Jun 13, 2017

Hawkular hostname detection changes #1304

Hawkular hostname detection changes #1304

Conversation

moolitayer commented May 7, 2017 • edited

moolitayer commented May 7, 2017

moolitayer commented May 8, 2017

moolitayer commented May 8, 2017

zeari commented May 8, 2017 • edited

zeari commented May 8, 2017

moolitayer commented May 8, 2017

moolitayer commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented May 9, 2017

dclarizio commented May 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented May 29, 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

cben commented May 29, 2017

moolitayer commented Jun 4, 2017

moolitayer commented Jun 5, 2017

himdel commented Jun 6, 2017 • edited

himdel commented Jun 6, 2017

himdel Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Jun 7, 2017

miq-bot commented Jun 7, 2017

himdel commented Jun 7, 2017 • edited

moolitayer commented Jun 7, 2017

himdel commented Jun 7, 2017

moolitayer commented Jun 7, 2017

moolitayer commented Jun 13, 2017

moolitayer commented May 7, 2017 •

edited

zeari commented May 8, 2017 •

edited

himdel commented Jun 6, 2017 •

edited

himdel Jun 6, 2017 •

edited

himdel commented Jun 7, 2017 •

edited