Convert containers hawkular Endpoints port=nil to port=443

[cben]

For a time (before ManageIQ/manageiq-ui-classic#1172), it was possible in UI to save containers provider with blank hawkular port, which got saved as port = nil.
Then it'd be impossible to edit the provider, as UI would crash:

On fine this was fixed by ManageIQ/manageiq-ui-classic#1565 adding special case in UI to tolerate nil.
On master, @AparnaKarve asked to not introduce special cases but do a migration.
Better late than never :-)

After investigating, @yaacov @moolitayer and me concluded such provider does connect to hawkular with port 443 (it builds a url without port, confirmed experimentally).
This migration normalizes such endpoints to port 443, works same but possible to edit in UI:

Tested by checking out fine-1, creating such provider, migrating all the way to master, it was still broken, and fixed by this migration. That's how I took above screenshots.
(in some cases such endpoints might have already been deleted by ManageIQ/manageiq#14990 migration. But not if hostname was autodetected from route => this is still sometimes needed.)

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

@miq-bot add-label bug, data
@moolitayer @AparnaKarve please review

[moolitayer]

This migration normalizes such endpoints to port 0, still invalid but possible to edit in UI:

@cben what if we remove these endpoints altogether? that way we will not have failed authentication checks for an endpoint the user did not intend to create (event if the user does not go into the edit screen and disables metrics altogether)

[cben]

These endpoints were only possible by actually going into Hawkular tab and clearing the port field, not sure it's "didn't intend to create" but never mind. Yeah, I'm fine with deleting too. Don't have an opinion, please decide for me :) cc @yaacov @zeari Deleting will result in UI showing "Metrics: disabled" in Provider Edit right? In provider details, will it show as metrics error (ideal IMHO), or not show metrics at all?

[yaacov]

it was possible in UI to save containers provider with blank hawkular port, which got saved as port = nil.

a. This is fine, when port==nil we fallback to default port [1] , IMHO, if you want to migrate it, it should be to the default port.

b. In old versions of ManageIQ it was not possible to have no metrics, metrics where always on, and values fall-back to default url. port and token if not given explicitly. IMHO migration of missing endpoint should be to a default values, and to nil/0.

[1] https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/master/app/models/manageiq/providers/kubernetes/container_manager/metrics_capture/hawkular_client_mixin.rb#L23

[Fryguy]

This looks good to me, I just want approval from @simon3z or @moolitayer before merge.

[simon3z]

a. This is fine, when port==nil we fallback to default port [1] , IMHO, if you want to migrate it, it should be to the default port.

+1 for what @yaacov is suggesting here.
@cben what do you think?

b. In old versions of ManageIQ it was not possible to have no metrics, metrics where always on, and values fall-back to default url. port and token if not given explicitly. IMHO migration of missing endpoint should be to a default values, and to nil/0.

This one is actually sad, but I don't have any other good suggestions here.

[moolitayer]

These endpoints were only possible by actually going into Hawkular tab and clearing the port field, not sure it's "didn't intend to create" but never mind.

I thought this was a situation where the user had pressed add from the default endpoint tab
I'm good with @yaacov's suggestion in this case 👍
I was really surprised that fine screens allowed that... (I checked that master blocks it so I can sleep at night)

Deleting will result in UI showing "Metrics: disabled" in Provider Edit right?

Right

[cben]

Confirmed experimentally, such endpoint is not broken as I originally thought but accesses https URL with port omitted, so it uses port 443. This is also what @yaacov @moolitayer and me saw in code.

=> Changed migration to set port 443. Dropped down migration, will not change 443 into nil.
Updated PR description and screenshot. PTAL.

[miq-bot]

Checked commit cben@dd5ed57 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

db/migrate/20171030131403_fix_hawkular_endpoints_with_port_nil.rb

• ❗ - Line 11, Col 54 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

[cben]

@yaacov PTAL

[yaacov]

what about the update_all , don't you want to fix that first ?

o/w LGTM 👍

[yaacov]

LGTM 👍

p.s.
unless you want to change update_all to something else

[cben]

you mean miq-bot? IIUC we routinely ignore this advice in migrations, because we want migrations to be fast where feasible, and because the model stubs don't normally have validations anyway.

@Fryguy please review (nice to have but not blocker)

[Fryguy]

yeah, ignore the update_all warning (I keep forgetting to change that in the rubocop.yml)

@moolitayer You good with this?

[chessbyte]

@simon3z @moolitayer can one of you guys approve this PR?

[moolitayer]

LGTM 👍