Adding options field to ext_management_system

[enoodle]

This will add the serialzed column options to ExtManagementSystem.
There is also a migration for ManageIQ::Providers::Kubernetes::ContainerManager options that where saved in CustomAttributes and general settings but should be there. This takes inspiration from #13826 and #14106 but the options details will be added in a later stage in the appropriate provider modules.

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1462835

[enoodle]

cc @simon3z @blomquisg

[simon3z]

@enoodle we don't want to move the Settings values in the options.

[simon3z]

@enoodle I don't think we want to rely on the Kubernetes module dependency here for this migration. cc @blomquisg

[enoodle]

I am not sure I understand, if we dont migrate these kubernetes options and not the ones from the settings, what should we migrate? On Jun 20, 2017 12:56 AM, "Federico Simoncelli" <[email protected]> wrote: *@simon3z* commented on this pull request. ------------------------------ In db/migrate/20170619161514_add_options_to_ext_management_system.rb <#15398 (comment)>:

+ serialize :serialized_value

+ end + + def up + add_column :ext_management_systems, :options, :text + + say_with_time("Migrating Kubernetes provider options") do + ExtManagementSystem.where(:type => "ManageIQ::Providers::Kubernetes::ContainerManager").each do |cp| + migrated_options = {} + migrated_options[:image_inspector_repository] = Settings.ems.ems_kubernetes.image_inspector_repository + migrated_options[:image_inspector_registry] = Settings.ems.ems_kubernetes.image_inspector_registry + CustomAttribute.where( + :resource_type => cp.type, + :resource_id => cp.id, + :section => ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job::ATTRIBUTE_SECTION, + :name => ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job::PROXY_ENV_VARIABLES).each do |ca| @enoodle <https://github.com/enoodle> I don't think we want to reply on the Kubernetes module dependency here for this migration. cc @blomquisg <https://github.com/blomquisg> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15398 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC-ogKxtxMuepzazq7RdtTYwaxaFyXH5ks5sFu5vgaJpZM4N-kbK> .

[simon3z]

I am not sure I understand, if we dont migrate these kubernetes options and
not the ones from the settings, what should we migrate?

@enoodle I don't want you to prepopulate the values in the db for the Settings ones... otherwise we can't change the defaults going forward.

Read more carefully the comment about the kubernetes options, it's about the module dependency.

[simon3z]

Current implementation adding a generic options json column to ext_management_system LGTM 👍.

Of course I am also considering the downside of this approach. The table authentications just recently added the same options json column so I assume we reached the same conclusion on the need for a more flexible field for special best-of-breed options (of course it must be used with care).

For completeness I want to mention that since most of the times (I'd say always?) these are also global settings, e.g.:

:ems:
  :ems_kubernetes:
    :miq_namespace: management-infra
    :image_inspector_registry: docker.io
    :image_inspector_repository: openshift/image-inspector

It's worth mentioning that this could be achieved also using ExtManagementSystem specific settings in the settings_changes table:

id	resource_type	resource_id	key	value
100	ExtManagementSystem	1	/ems/ems_kubernetes/image_inspector_registry	mydockerregistry.io
101	ExtManagementSystem	2	/ems/ems_kubernetes/image_inspector_registry	myotherdockerregistry.io

I am not sure of the implications of this second approach though.

cc @blomquisg

[simon3z]

@enoodle as mentioned before I am not sure if we can afford this dependency to the Kubernetes module. cc @blomquisg You may have to hard-code the values here.

[enoodle]

I have added deletion of the old CustomAttribute's with the settings and also made sure that the migration is happening on Kubernetes and Openshift Providers.

[simon3z]

@enoodle isn't it risky to delete this before having it persisted in the new options? (cp.update)

[simon3z]

@blomquisg @Fryguy can you review?

[miq-bot]

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

[Fryguy]

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

[miq-bot]

Checked commit enoodle@ad37b0e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[moolitayer]

cc @Loicavenel