Remove Invalid Hawkular Endpoints #14990

moolitayer · 2017-05-04T07:18:49Z

Depends on: ManageIQ/manageiq-ui-classic#1205

Before this migration a disabled hawkular is expressed by an endpoint with a nil hostname.
After, a provider with a disabled hawkular will only have the default endpoint.

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

moolitayer · 2017-05-08T13:37:07Z

cc @cben @zeari @simon3z

cben · 2017-05-09T08:11:44Z

db/migrate/20170427152006_remove_invalid_hawkular_endpoints.rb

+    Endpoint.where(
+      :resource_type => "ExtManagementSystem",
+      :role          => 'hawkular',
+      :hostname      => nil


can it also be blank string?

nil in the bug description[1] but it's a good idea to test both options

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1437138#c19

cben · 2017-05-09T08:13:56Z

db/migrate/20170427152006_remove_invalid_hawkular_endpoints.rb

+    ExtManagementSystem.where(
+      :type => %w(ManageIQ::Providers::Openshift::ContainerManager ManageIQ::Providers::Kubernetes::ContainerManager)
+    ).each do |ems|
+      Endpoint.create!(


what about providers that have both endpoints?
wouldn't this create a 3rd one going back?

Right, updated

cben

what about the Authentication associated with hawkular?
I believe we should delete it too, to preserve 1:1 correspondence we had till now.

moolitayer · 2017-05-09T10:47:54Z

@miq-bot add_label fine/yes

cben · 2017-05-09T10:57:54Z

isn't fine schema frozen?

moolitayer · 2017-05-09T11:19:15Z

isn't fine schema frozen?

I hope this will be considered for the next zstream, especially since It is an update and not a schema change(ddl). If it is rejected we will have a separate change for fine (probably in manageiq-ui-classic) and this one for master

simon3z · 2017-05-09T12:10:46Z

@moolitayer @cben currently my vote is to have no migrations (of any kind) in z-stream.

I'll leave to @Fryguy to approve a special exception if he thinks otherwise.

moolitayer · 2017-05-10T17:12:12Z

Lets leave the label for later
@cben @simon3z please review

@miq-bot remove_label fine/yes

cben · 2017-05-11T11:55:29Z

db/migrate/20170427152006_remove_invalid_hawkular_endpoints.rb

+  def up
+    Endpoint.where(
+      :resource_type => "ExtManagementSystem",
+      :role          => 'hawkular',


are we sure say middleware providers don't use this role?
perhaps constrain EMS types like you did in down.

They are using only the default[1].
Since it saves a join I prefer it.

[1] https://github.com/manageiq/manageiq-ui-classic/blob/deadce343f4105142a56e07cae3aab89a9d9151b/app/controllers/mixins/ems_common_angular.rb#L443-L446

cben · 2017-05-11T12:01:36Z

Code looks plausible, but see comment above about Authentication.

Have you tested by actually creating providers on old releases and migrating forward? I've done that before, can help.
Can this benefit from a spec? I'm not sure what exactly to test.

moolitayer · 2017-05-11T13:49:13Z

Have you tested by actually creating providers on old releases and migrating forward? I've done that before, can help.

Yes I have, added a spec

moolitayer · 2017-05-16T10:16:02Z

@Fryguy please review this change

moolitayer · 2017-05-16T10:16:51Z

@cben @yaacov please review

cben

👍 ♻️
I'm really glad that you're digging us out of the "unofficially optional hawkular" mess...

cben · 2017-05-17T08:44:18Z

spec/migrations/20170427152006_remove_invalid_hawkular_endpoints_spec.rb

+        :resource_id   => ems.id
+      )
+      migrate
+      expect(endpoint_stub.count).to eq(1)


I don't really expect it to remove the wrong one, but still maybe test which one got removed...
E.g. expect(endpoint_stub.pluck(:role)).to contain_exactly("default")

simon3z · 2017-05-17T13:27:39Z

LGTM 👍
cc @Fryguy

moolitayer · 2017-05-18T02:31:34Z

@Fryguy please reivew

moolitayer · 2017-05-25T09:45:58Z

@Fryguy please review

moolitayer · 2017-05-25T13:23:51Z

Added deletion of the Hawkular authentication. That required some changes since polymorphic associations do not work well with migration stubs.

@cben @Fryguy please review

cben · 2017-05-28T13:17:46Z

db/migrate/20170427152006_remove_invalid_hawkular_endpoints.rb

+      :resource_id   => ems_container_ids,
+      :role          => "hawkular",
+    )
+    create_ids = ems_container_ids - ems_with_hawkular


does [ids] - [records] work?
Id add .pluck(:id) anyway

actually ems_with_hawkular will be Endpoint, so pluck :resource_id ?

miq-bot · 2017-05-28T13:27:52Z

Checked commit moolitayer@865b040 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

cben · 2017-05-28T14:51:55Z

LGTM

moolitayer · 2017-05-28T15:42:05Z

@cben much appreciated
@Fryguy Please review

chessbyte · 2017-06-02T04:00:31Z

@Fryguy bump

moolitayer · 2017-06-04T07:16:58Z

@Fryguy please review

moolitayer · 2017-06-05T10:16:20Z

@Fryguy please review

moolitayer · 2017-06-11T15:17:30Z

@Fryguy please review

moolitayer · 2017-06-13T13:59:55Z

@miq-bot add_label fine/no

Fryguy · 2017-06-13T18:00:35Z

db/migrate/20170427152006_remove_invalid_hawkular_endpoints.rb

+  def down
+    ems_containers_by_id = ExtManagementSystem.where(
+      :type => %w(ManageIQ::Providers::Openshift::ContainerManager ManageIQ::Providers::Kubernetes::ContainerManager)
+    ).map { |ems| [ems.id, ems] }.to_h


Minor, but .map { |ems| [ems.id, ems] }.to_h can be shortened to .index_by(&:id)

moolitayer mentioned this pull request May 4, 2017

Allow to explicitly disable hawkular in containers ManageIQ/manageiq-ui-classic#1205

Merged

miq-bot added the sql migration label May 4, 2017

moolitayer force-pushed the migrate_endpoint branch from aa3b963 to 2d32ab0 Compare May 4, 2017 12:26

chessbyte assigned Fryguy May 8, 2017

chessbyte added providers/containers enhancement labels May 8, 2017

cben reviewed May 9, 2017

View reviewed changes

miq-bot added the fine/yes label May 9, 2017

moolitayer force-pushed the migrate_endpoint branch 3 times, most recently from 1169423 to 2f44340 Compare May 9, 2017 11:14

moolitayer force-pushed the migrate_endpoint branch 2 times, most recently from 52c81d1 to 09791d2 Compare May 9, 2017 11:29

miq-bot removed the fine/yes label May 10, 2017

cben reviewed May 11, 2017

View reviewed changes

moolitayer force-pushed the migrate_endpoint branch 2 times, most recently from 4783a56 to faa2ee4 Compare May 11, 2017 13:48

moolitayer force-pushed the migrate_endpoint branch from faa2ee4 to 4d79179 Compare May 11, 2017 21:53

cben approved these changes May 17, 2017

View reviewed changes

simon3z approved these changes May 17, 2017

View reviewed changes

yaacov approved these changes May 25, 2017

View reviewed changes

moolitayer force-pushed the migrate_endpoint branch from 4d79179 to 2456338 Compare May 25, 2017 13:22

moolitayer force-pushed the migrate_endpoint branch 3 times, most recently from a2d968d to 6bcc22b Compare May 28, 2017 11:00

cben reviewed May 28, 2017

View reviewed changes

moolitayer force-pushed the migrate_endpoint branch from 6bcc22b to f0849f4 Compare May 28, 2017 13:21

Remove Invalid Hawkular Endpoints

865b040

moolitayer force-pushed the migrate_endpoint branch from f0849f4 to 865b040 Compare May 28, 2017 13:23

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

Fryguy reviewed Jun 13, 2017

View reviewed changes

Fryguy merged commit d8687aa into ManageIQ:master Jun 13, 2017

Fryguy added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 13, 2017

cben mentioned this pull request Oct 18, 2017

Convert containers hawkular Endpoints port=nil to port=443 ManageIQ/manageiq-schema#98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Invalid Hawkular Endpoints #14990

Remove Invalid Hawkular Endpoints #14990

moolitayer commented May 4, 2017 •

edited

Loading

moolitayer commented May 8, 2017

cben May 9, 2017

moolitayer May 9, 2017

cben May 9, 2017

moolitayer May 9, 2017

cben left a comment

moolitayer commented May 9, 2017

cben commented May 9, 2017

moolitayer commented May 9, 2017

simon3z commented May 9, 2017

moolitayer commented May 10, 2017

cben May 11, 2017

moolitayer May 11, 2017

cben commented May 11, 2017

moolitayer commented May 11, 2017

moolitayer commented May 16, 2017

moolitayer commented May 16, 2017

cben left a comment

cben May 17, 2017

simon3z commented May 17, 2017

moolitayer commented May 18, 2017

moolitayer commented May 25, 2017

moolitayer commented May 25, 2017

cben May 28, 2017

cben May 28, 2017

miq-bot commented May 28, 2017

cben commented May 28, 2017

moolitayer commented May 28, 2017

chessbyte commented Jun 2, 2017

moolitayer commented Jun 4, 2017

moolitayer commented Jun 5, 2017

moolitayer commented Jun 11, 2017

moolitayer commented Jun 13, 2017

Fryguy Jun 13, 2017

Remove Invalid Hawkular Endpoints #14990

Remove Invalid Hawkular Endpoints #14990

Conversation

moolitayer commented May 4, 2017 • edited Loading

moolitayer commented May 8, 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 left a comment

Choose a reason for hiding this comment

moolitayer commented May 9, 2017

cben commented May 9, 2017

moolitayer commented May 9, 2017

simon3z commented May 9, 2017

moolitayer commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented May 11, 2017

moolitayer commented May 11, 2017

moolitayer commented May 16, 2017

moolitayer commented May 16, 2017

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented May 17, 2017

moolitayer commented May 18, 2017

moolitayer commented May 25, 2017

moolitayer commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 28, 2017

cben commented May 28, 2017

moolitayer commented May 28, 2017

chessbyte commented Jun 2, 2017

moolitayer commented Jun 4, 2017

moolitayer commented Jun 5, 2017

moolitayer commented Jun 11, 2017

moolitayer commented Jun 13, 2017

Choose a reason for hiding this comment

moolitayer commented May 4, 2017 •

edited

Loading