Fix adding Kubernetes provider

[cben]

Reported by @evertmulder yesterday, also encountered by @AparnaKarve and by me today.
If get_hostname_from_routes causes exception, you can't [Validate] nor [Add]/[Save] the provider.
This was especially a problem for Kubernetes, apparently made it impossible to create Kubernetes provider via UI.
(Not sure if recent #265 made it more exception-prone? I can confirm the guessing does work on Openshift.)

• Skipping the guessing entirely for Kubernetes because Routes are an Openshift concept.

IIUC this functionality should be best-effort, failure to guess the hawkular route should not stop block user (if they can make validation happy otherwise).

(The error also doesn't get to UI, the 500 JS page is not the JSON the UI was looking for. That could be improved but in this case I believe errors are not interesting to user anyway.)

@simon3z Please review.

@miq-bot add-label providers/containers, bug

[simon3z]

@enoodle @moolitayer please review

[simon3z]

@miq-bot assign cben

[cben]

@yaacov welcome back, can you review?

[yaacov]

LGTM 👍

[cben]

Confirmed, it's currently impossible to add Kubernetes provider via UI!
Editing an existing one is probably impossible too.

Checking euwe...

[enoodle]

LGTM, one comment though:

I know that we are not collecting them yet, but Kubernetes have introduced a new object called ingress that is a bit similar to Openshift Routes [1]. Maybe we can add a TODO here to remember to check with the ingress once we collect them (Possibly we will collect ingress under the route entity in ManageIQ)

[1]https://kubernetes.io/docs/user-guide/ingress/

[moolitayer]

LGTM 👍

[cben]

I know that we are not collecting them yet, but Kubernetes have introduced a new object called ingress that is a bit similar to Openshift Routes [1]. Maybe we can add a TODO here to remember to check with the ingress once we collect them (Possibly we will collect ingress under the route entity in ManageIQ)

This is direct request, independent of refresh (it can run before adding the provider). So we could do it right now, assuming Kubeclient supports it. But not having the route API was just a symptom, I believe people don't normally run hawkular-metrics at on kubernetes... (Or at least it's highly atypical; if one does it explicitly filling hostname in Hawkular endpoint should still work.)

[cben]

euwe is unaffected. @miq-bot add-label euwe/no

[simon3z]

@cben please add tests

[miq-bot]

Checked commit cben@7b6fe36 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 2 offenses detected

spec/controllers/ems_common_controller_spec.rb

• ❗ - Line 192, Col 5 - Metrics/BlockLength - Block has too many lines. [31/25]
• ❕ - Line 237, Col 9 - Metrics/AbcSize - Assignment Branch Condition size for test_setting_many_fields is too high. [47/20]

[cben]

Added tests, @yaacov please review.

More integration than unit test, because I have hopes to refactor this route fetching to later point when the EMS is already built, and I don't want tests to depend on current interface.
The tests talking about "update" vs "adding" is a lie because they just test set_ems_record_vars. Ideally should do full post :button, :params => ... tests, for all new/edit X validate/hawk validate/save combinations, but out of scope for this PR.

@yaacov the tests call set_ems_record_vars twice on same object, in what I now split into test_setting_many_fields then test_setting_few_fields. You added the second call in
6c63ea7#diff-a3ccea15493bac675c667462322d3047R195
IIUC that never happens in real life — when creating every button click creates a new EMS instance, when editing [Validate] click does find_by_id_filtered but never saves the instance.
I tried splitting the 2nd call to work on fresh EMS instance but that failed 'valid-token' assertions assuming it survived from 1st call.
Is this deliberate?

Consciously ignoring Rubocop here. There are bigger issues with these tests but this is a step forward.

[close-cycling to kick travis, bower EINCOMPLETE]

[yaacov]

LGTM 👍

[cben]

@miq-bot assign dclarizio

[martinpovolny]

Well I have to ask why we have so much provider-specific code in a mixin that should have contained the common code for all the providers. But that is not the fault of this PR.