Added a new column to store embedded method names #14847
Conversation
|
@Fryguy please review |
| @@ -0,0 +1,5 @@ | |||
| class AddEmbeddedMethodsToMiqAeMethod < ActiveRecord::Migration[5.0] | |||
| def change | |||
| add_column :miq_ae_methods, :embedded_methods, :string, array: true, default: [] | |||
There was a problem hiding this comment.
I think you want default_value_for in the model, not :default in the migration.
app/models/miq_ae_method.rb
Outdated
| @@ -3,6 +3,8 @@ | |||
| class MiqAeMethod < ApplicationRecord | |||
| include MiqAeSetUserInfoMixin | |||
| include MiqAeYamlImportExportMixin | |||
| serialize :embedded_methods, Array | |||
There was a problem hiding this comment.
You shouldn't have to serialize this because it's an Array column
| @@ -0,0 +1,5 @@ | |||
| class AddEmbeddedMethodsToMiqAeMethod < ActiveRecord::Migration[5.0] | |||
| def change | |||
| add_column :miq_ae_methods, :embedded_methods, :string, :array => true | |||
There was a problem hiding this comment.
How long could the data be? Should it be :text instead of :string?
There was a problem hiding this comment.
@bdunne It contains the method names and its possible that we could have lengths longer than 255 if they have a handful of methods defined. I have changed it to a text field.
Changed to use text instead of string Removed the serialize
|
@Fryguy |
| @@ -0,0 +1,5 @@ | |||
| class AddEmbeddedMethodsToMiqAeMethod < ActiveRecord::Migration[5.0] | |||
| def change | |||
| add_column :miq_ae_methods, :embedded_methods, :text, :array => true | |||
There was a problem hiding this comment.
@bdunne @Fryguy
For existing records #embedded_methods will return a nil but if we set the default value (:default => []) in the add_column field #embedded_methods will return a [].
That means in the code we don't have to check for nil values.
So my question is along with the model default_value_for should we set the add_column default.
There was a problem hiding this comment.
@mkanoor It's my understanding that we avoid using postgres defaults for all columns and leave all of the default logic in ruby. So, you can add a line to this migration to populate the column on all existing records.
|
Checked commits mkanoor/manageiq@24e595f~...3da21cb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 db/migrate/20170421193150_add_embedded_methods_to_miq_ae_method.rb
|
A new enhancement to add embedded method names to a miq_ae_method table.
To support changes in #14286