Add options to ems #23
Conversation
enoodle
force-pushed
the
add_options_to_ems
branch
2 times, most recently
from
4f85b51
to
88db4df
Compare
enoodle
force-pushed
the
add_options_to_ems
branch
from
88db4df
to
658c31b
Compare
|
@simon3z @moolitayer @Fryguy Please take a look |
| add_column :ext_management_systems, :options, :text | ||
|
|
||
| say_with_time("Migrating Kubernetes provider options") do | ||
| ExtManagementSystem.where(:type => ["ManageIQ::Providers::Kubernetes::ContainerManager", |
There was a problem hiding this comment.
I think we can remove N+1 and do something like:
ExtManagementSystem.where(...).includes(:custom_attributes).where(:custom_attributes => {:section=> 'cluster_settings',... }.each do |ems|
options = ems.custom_attributes.map { |ca| Hash[*["image_inspector_#{ca.name}", ca.value]] }
ems.update(:options => options)
endDoes it cover your intention?
| end | ||
|
|
||
| def up | ||
| add_column :ext_management_systems, :options, :text |
There was a problem hiding this comment.
There was a problem hiding this comment.
We are serializing this field. Does json work well with that? We are trying to mimic https://github.com/ManageIQ/manageiq-schema/blob/master/db/migrate/20170206210552_add_options_column_to_authentication.rb
There was a problem hiding this comment.
I get that, I think the code should work the same except for dropping the serialize and some other code in the tests.
That way we get native JSON support (for example query only the http_proxy out of options) in the db layer.
@durandom wdyt?
There was a problem hiding this comment.
we have jsonb in our migrations, e.g. showback lately
maybe look at their ActiveRecord model about serializing. I have never looked at how we do it
There was a problem hiding this comment.
Downside to JSON is that you don't get time objects. Will you need those?
There was a problem hiding this comment.
Maybe we will want to have time based optoins? @simon3z
There was a problem hiding this comment.
OK I understand that gap in data types. Since this one is meant to be small
(configuration) I think clear serialization is more important then efficiency.
So I change my mind 👍 for :text
There was a problem hiding this comment.
Maybe we will want to have time based optoins? @simon3z
+1 for the ability to have time objects
enoodle
force-pushed
the
add_options_to_ems
branch
from
658c31b
to
3f1b32e
Compare
| @@ -19,7 +19,6 @@ def up | |||
| ExtManagementSystem | |||
| .where(:type => ["ManageIQ::Providers::Kubernetes::ContainerManager", | |||
| "ManageIQ::Providers::Openshift::ContainerManager"]) | |||
| .includes(:custom_attributes).where(:custom_attributes => {:section => "cluster_settings"}) | |||
There was a problem hiding this comment.
I wonder why this line causes this error: PG::UndefinedColumn: ERROR: column ext_management_systems.security_protocol does not exist , Can anyone advise? cc @lpichler
| serialize :options | ||
| end | ||
|
|
||
| class CustomAttribute < ActiveRecord::Base |
There was a problem hiding this comment.
We now put an inheritance_column in every mock class to make it future proof
enoodle
force-pushed
the
add_options_to_ems
branch
from
a1393ee
to
ba97008
Compare
| cp.custom_attributes.where(:section => 'cluster_settings', | ||
| :name => %w(no_proxy http_proxy https_proxy)) | ||
| .map { |ca| ["image_inspector_#{ca.name}".to_sym, ca.value] }] | ||
| cp.update(:options => migrated_options) |
There was a problem hiding this comment.
the custom attribute names are cluster_settings/no_proxy cluster_settings/http_proxy etc and the new one is image_inspector_no_proxy ?
I think that would be better expressed with nesting.
| end | ||
|
|
||
| def down | ||
| remove_column :ext_management_systems, :options |
There was a problem hiding this comment.
up & down will remove the setting...
I'm not sure how stringent we need to be here
|
|
||
| migration_context :up do | ||
| it "up" do | ||
| kcp = ems_stub.create!(:type => "ManageIQ::Providers::Openshift::ContainerManager", :id => 1) |
There was a problem hiding this comment.
We should avoid ids in tests - that violates region constraints
enoodle
force-pushed
the
add_options_to_ems
branch
2 times, most recently
from
236d34b
to
fc194a0
Compare
|
Checked commit enoodle@fc194a0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 db/migrate/20170619161514_add_options_to_ext_management_system.rb
spec/migrations/20170619161514_add_options_to_ext_management_system_spec.rb
|
|
@miq-bot assign Fryguy |
|
ping @Fryguy |
|
cc @Loicavenel |
This is migration of ManageIQ/manageiq#15398