Check metrics details from `raw_connect` #134

jhernand · 2017-11-07T12:20:39Z

Currently the raw_connect doesn't check the metrics connection
details. This patch adds that check.

This patch fixes partially the following bug:

RHV provider metrics validation is broken
https://bugzilla.redhat.com/1510533

jhernand · 2017-11-07T12:22:25Z

@pkliczewski @borod108 @masayag please review.

Take into account that this pull request should be merged after #132. This currently contains two commits, the first is also in that other pull request. So please focus only in the second commit of this pull request.

jhernand · 2017-11-07T12:23:22Z

Note also that for this to work the UI has to be modified to send the required data. That is pull request ManageIQ/manageiq-ui-classic#2651.

pkliczewski · 2017-11-07T13:39:21Z

@jhernand This PR requires few changes in vcr files. For now the specs are failing.

miq-bot · 2017-11-07T14:37:27Z

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

jhernand · 2017-11-07T15:08:47Z

Bug 1509432 - Failed validation when adding RHV provider

masayag · 2017-11-07T15:55:48Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+ rescue StandardError => error
+ raise adapt_metrics_error(error)
+ ensure
+ OvirtMetrics.disconnect rescue nil


seems that rubocop would prefer

begin OvirtMetrics.disconnect rescue nil end

but we have the same style in line 145

I will do as Rubocop says.

oourfali · 2017-11-07T15:57:38Z

@miq-bot add_label gaprindashvili/yes

borod108 · 2017-11-07T14:55:03Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+ # @options opts [String] :metrics_database The name of the metrics database.
+ # @return [Boolean] Returns `true` if the connection details and credentials are valid, or `false` otherwise.
+ #
+ def raw_connect(opts = {})


is there anyway we can rename this to something with "validate?"

No, this is the name that the UI uses, it has to be raw_connect.

borod108 · 2017-11-07T14:56:00Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+ #
+ def raw_connect(opts = {})
+ check_connect_api(opts)
+ check_connect_metrics(opts)


why are we checking both of these? how do we know which one failed?

We need to check all the details that the user gives in the UI, if I understand correctly. We won't know what failed, nor do we need: the human user will know, hopefully, using her eyes and brain.

Currently the `raw_connect` doesn't check the metrics connection details. This patch adds that check. This patch fixes partially the following bug: Failed validation when adding RHV provider https://bugzilla.redhat.com/1509432

miq-bot · 2017-11-07T16:04:57Z

Checked commit https://github.com/jhernand/manageiq-providers-ovirt/commit/1a6e4dc50f1c4644f065ebdc613c277fd1579680 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

borod108 · 2017-11-07T16:28:39Z

Well yes, we can safely assume the user has eyes and brain :) So using his eyes and brain, will he be able to see which if the credentials fail? I mean we have it in two tabs, and the validation button from the code here looks like it sends the info from both, does it show in which tab the failure is? Its a UI question, I know, I am just wondering because I seem to remember it did not work so well. Any way I am for merging, just wondering...

…

On Tue, Nov 7, 2017 at 6:04 PM, Juan Hernández ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/models/manageiq/providers/redhat/infra_manager/api_integration.rb <#134 (comment)> : > + # @option opts [Integer] :port ('443') The port number of the API server. + # @option opts [Integer] :verify_ssl ('1') A numeric flag indicating if the TLS certificates of the API server + # should be checked. Value `0` indicates that the should not be checked, value `1` indicates that they should + # be checked. + # @option opts [String] :ca_certs The custom trusted CA certificates used to check the TLS certificates of the + # API server, in PEM format. A blank or nil value means that no custom CA certificates should be used. + # @option opts [String] :metrics_userid The name of the metrics database user. + # @option opts [String] :metrics_password The password of the metrics database user. + # @options opts [String] :metrics_server The host name or IP address of the metrics database server. + # @options opts [Integer] :metrics_port ('5432') The port number of the metrics database server. + # @options opts [String] :metrics_database The name of the metrics database. + # @return [Boolean] Returns `true` if the connection details and credentials are valid, or `false` otherwise. + # + def raw_connect(opts = {}) + check_connect_api(opts) + check_connect_metrics(opts) We need to check all the details that the user gives in the UI, if I understand correctly. We won't know what failed, nor do we need: the human user will know, hopefully, using her eyes and brain. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH365fNgzz3TlO9l5MGbcGgl5jWo63Xks5s0H-EgaJpZM4QUtD5> .

jhernand · 2017-11-07T16:33:35Z

@borod108 in my tests the error messages displayed to the user were very informative, and it was easy to determine what was wrong. There is certainly room for improvement, but I really don't want to mix those improvements with fixing the main issue.

jhernand · 2017-11-07T16:55:42Z

The tests are failing with this error:

     NoMethodError:
       undefined method `manager_ref' for #<ManageIQ::Providers::Redhat::InfraManager::Vm:0x0000000b45efd8>
       Did you mean?  manager_type
     # /home/travis/.rvm/gems/ruby-2.3.1/gems/activemodel-5.0.6/lib/active_model/attribute_methods.rb:433:in `method_missing'
     # ./spec/manageiq/app/models/manager_refresh/target_collection.rb:41:in `collect'
     # ./spec/manageiq/app/models/manager_refresh/target_collection.rb:41:in `id'
     # ./spec/manageiq/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb:23:in `block (2 levels) in refresh'
     # ./spec/manageiq/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb:23:in `each'
     # ./spec/manageiq/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb:23:in `block in refresh'
     # ./spec/manageiq/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb:14:in `each'
     # ./spec/manageiq/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb:14:in `refresh'
     # ./spec/manageiq/app/models/manageiq/providers/base_manager/refresher.rb:9:in `refresh'
     # ./spec/manageiq/app/models/ems_refresh.rb:97:in `block in refresh'
     # ./spec/manageiq/app/models/ems_refresh.rb:96:in `each'
     # ./spec/manageiq/app/models/ems_refresh.rb:96:in `refresh'
     # ./spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_4_spec.rb:147:in `block (2 levels) in <top (required)>'

I think that isn't related to this change, but rather to ManageIQ/manageiq#16405.

@Ladas @agrare @pkliczewski can you take a look. I think that the reason is that the above pull request assumes that all targeted objects have a manager_ref attribute, but virtual machines don't have it, at least not oVirt virtual machines.

jhernand · 2017-11-07T18:28:48Z

The fix for the issue that caused the test failure (ManageIQ/manageiq#16411) has been merged. I am closing and re-opening this to re-run the tests.

agrare · 2017-11-09T19:09:10Z

@simaishi don't backport this until #140 is also merged

…_connect Check metrics details from `raw_connect` (cherry picked from commit af9c776) https://bugzilla.redhat.com/show_bug.cgi?id=1511079

simaishi · 2017-11-14T23:42:58Z

Gaprindashvili backport details:

$ git log -1
commit 0e4e22fea0e8d737ed6a8184137ee0ebf64cd84d
Author: Moti Asayag <masayag@redhat.com>
Date:   Wed Nov 8 09:09:21 2017 +0200

    Merge pull request #134 from jhernand/verify_metrics_details_from_raw_connect
    
    Check metrics details from `raw_connect`
    (cherry picked from commit af9c776d98752f8274d08e21b36163bff555e556)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1511079

miq-bot added the unmergeable label Nov 7, 2017

miq-bot removed the unmergeable label Nov 7, 2017

masayag approved these changes Nov 7, 2017

View reviewed changes

pkliczewski approved these changes Nov 7, 2017

View reviewed changes

masayag reviewed Nov 7, 2017

View reviewed changes

miq-bot added the gaprindashvili/yes label Nov 7, 2017

borod108 reviewed Nov 7, 2017

View reviewed changes

Check metrics details from raw_connect

1a6e4dc

Currently the `raw_connect` doesn't check the metrics connection details. This patch adds that check. This patch fixes partially the following bug: Failed validation when adding RHV provider https://bugzilla.redhat.com/1509432

borod108 approved these changes Nov 7, 2017

View reviewed changes

jhernand closed this Nov 7, 2017

jhernand reopened this Nov 7, 2017

masayag merged commit af9c776 into ManageIQ:master Nov 8, 2017

agrare mentioned this pull request Nov 9, 2017

Fix credential validation if no metrics given #140

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check metrics details from `raw_connect` #134

Check metrics details from `raw_connect` #134

jhernand commented Nov 7, 2017 •

edited

Loading

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

pkliczewski commented Nov 7, 2017

miq-bot commented Nov 7, 2017

jhernand commented Nov 7, 2017

masayag Nov 7, 2017

jhernand Nov 7, 2017

oourfali commented Nov 7, 2017

borod108 Nov 7, 2017

jhernand Nov 7, 2017

borod108 Nov 7, 2017

jhernand Nov 7, 2017

miq-bot commented Nov 7, 2017

borod108 commented Nov 7, 2017 via email

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

agrare commented Nov 9, 2017

simaishi commented Nov 14, 2017

Check metrics details from raw_connect #134

Check metrics details from raw_connect #134

Conversation

jhernand commented Nov 7, 2017 • edited Loading

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

pkliczewski commented Nov 7, 2017

miq-bot commented Nov 7, 2017

jhernand commented Nov 7, 2017

masayag Nov 7, 2017

Choose a reason for hiding this comment

jhernand Nov 7, 2017

Choose a reason for hiding this comment

oourfali commented Nov 7, 2017

borod108 Nov 7, 2017

Choose a reason for hiding this comment

jhernand Nov 7, 2017

Choose a reason for hiding this comment

borod108 Nov 7, 2017

Choose a reason for hiding this comment

jhernand Nov 7, 2017

Choose a reason for hiding this comment

miq-bot commented Nov 7, 2017

borod108 commented Nov 7, 2017 via email

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

jhernand commented Nov 7, 2017

agrare commented Nov 9, 2017

simaishi commented Nov 14, 2017

Check metrics details from `raw_connect` #134

Check metrics details from `raw_connect` #134

jhernand commented Nov 7, 2017 •

edited

Loading