-
Notifications
You must be signed in to change notification settings - Fork 357
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
Hawkular hostname detection changes #1304
Conversation
@miq-bot add_label wip |
b66619d
to
734fe6f
Compare
@miq-bot remove_label wip |
@zeari can you please review this change? |
|
@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 |
The detect button will be grayed out if hostname and or port weren't inserted |
@cben can you review these changes? |
return nil unless ems.class.respond_to?(:openshift_connect) | ||
endpoint = Endpoint.new(endpoint_hash) | ||
# TODO: move to backend | ||
def get_hostname_from_routes(ems) |
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.
@moolitayer Can you move this method and all the other container
specific code in this PR to ems_container_controller
?
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.
Done
@miq-bot add_label compute/containers, enhancement, fine/yes |
c0a667a
to
549a24f
Compare
@moolitayer can you please review the codeclimate issues, some look easy to fix? Thx, Dan It may be just me, but when does the 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? |
miqService.sparkleOn(); | ||
miqService.detectWithRest($event, $scope.actionUrl) | ||
.then(function success(data) { | ||
$scope.$apply(function() { |
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.
Suddenly 4 space indent? ;)
@@ -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'); |
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.
and here
@@ -77,6 +77,16 @@ | |||
"checkchange" => "", | |||
"prefix" => prefix.to_s, | |||
"reset-validation-status" => "#{prefix}_auth_status"} | |||
- detect = "detectClicked({target: '.detect_button'}, true)" |
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.
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.
@@ -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, |
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.
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.
f1b0f9f
to
10f005a
Compare
@@ -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); |
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.
@himdel is there a better way for the above?
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.
@moolitayer You could use
angular.element('#hawkular_hostname').val(value);
which is also recommended by CodeClimate.
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.
Thanks @AparnaKarve
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.
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...
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.
no reason to access it magically
Sure, if just the assignment $scope.emsCommonModel.hawkular_hostname = value
works, why not?
10f005a
to
82ba9c4
Compare
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. |
%span | ||
%miq-button.detect_button{"ng-if" => detection ? 'true' : 'false', | ||
:name => _("Detect"), | ||
:on_click => detect, |
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.
@himdel these attribute names seem to work.
Do you want them changed to strings with -?
(:on_click, :disabled_title, :enabled_title:)
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.
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..
$scope.detectClicked = function($event) { | ||
miqService.sparkleOn(); | ||
miqService.detectWithRest($event, $scope.actionUrl) | ||
.then(function success(data) { |
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.
perhaps should also have a failure callback? (to display a flash)
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 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.
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), |
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.
hooray :-)
:name => _("Detect"), | ||
"on-click" => detect, | ||
:enabled => "isDetectionEnabled()", | ||
"disabled-title" => _("Detection not available for provider type"), |
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 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.
2 tiny comments but this has long Looked Good To Me... Let me know if you want me to try it out. |
9bff73f
to
def03e5
Compare
@himdel please take a look |
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' || " + | |
this.detectWithRest = function($event, url) { | ||
angular.element('#button_name').val('detect'); | ||
miqSparkleOn(); | ||
return miqRESTAjaxButton(url, $event.target, 'json'); |
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.
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 ;)
miqService.sparkleOn(); | ||
miqService.detectWithRest($event, $scope.actionUrl) | ||
.then(function success(data) { | ||
$scope.$apply(function() { |
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.
..And you can remove this $scope.$apply
:) (https://github.com/ManageIQ/manageiq-ui-classic/pull/1304/files#r120332564)
"enabled-title" => _("Detect Endpoint"), | ||
:xs => 'true', | ||
:primary => 'true'} | ||
|
||
%span.help-block{"ng-show" => "angularForm.#{prefix}_hostname.$error.required"} |
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.
(these error %span
s need to be siblings of the %input
, not cousins) (#1304 (comment))
def03e5
to
9b8375f
Compare
Thanks for the catch @himdel, seems fine now: kindly rereview |
9b8375f
to
9c30024
Compare
Checked commit moolitayer@9c30024 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@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 Maybe make that |
Detection isn't available if a required filed is missing OR the provider type is kubernetes. 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 ) |
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? |
yes to remove the hawkular option when kubernetes is selected |
@miq-bot add_label fine/no |
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