Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conversion_host_id to miq_request_task #281

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
class AddConversionHostIdToMiqRequestTasks < ActiveRecord::Migration[5.0]
class MiqRequestTask < ActiveRecord::Base
self.inheritance_column = :_type_disabled
serialize :options, Hash
belongs_to :conversion_host, :class_name => "AddConversionHostIdToMiqRequestTasks::ConversionHost"
This conversation was marked as resolved.
Show resolved Hide resolved
end

class ConversionHost < ActiveRecord::Base
self.inheritance_column = :_type_disabled
belongs_to :resource, :polymorphic => true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can use polymorphic relations with the stub models. If you do, you'll get something like: :resource_type => MigrationClass::StubModel which won't be found in production.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right I thought in production it would use the type from the record but appears not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, who wants to take on changing this? Or should we write a follow up migration to alter all the broken resources to the "production ready" name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, should be able to just edit this migration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to update the .where to .in_my_region.find_each anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Jason would say we need a new migration since developers may have already run this one...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay #284 data migration to fix the resource_type

end

class Host < ActiveRecord::Base
self.inheritance_column = :_type_disabled
end

def up
add_column :miq_request_tasks, :conversion_host_id, :bigint

MiqRequestTask.where(:type => 'ServiceTemplateTransformationPlanTask').each do |task|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we prefer find_each to load records in batches.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this should probably be scoped with in_my_region to avoid running the migration on more records than necessary.

host_id = task.options.delete(:transformation_host_id)
next unless host_id
host = Host.find_by(:id => host_id)
carbonin marked this conversation as resolved.
Show resolved Hide resolved
if host.present?
task.conversion_host = ConversionHost.find_or_create_by!(:resource => host) do |ch|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConversionHost.find_or_initialize_by(:resource_type => "Host", :resource_id => host.id).update_attributes!(.....)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we should use find_or_initialize_by + update_attributes over what's here, but yeah it does look like this will break because the host class will be incorrect.

ch.name = host.name
ch.vddk_transport_supported = true
ch.ssh_transport_supported = false
end
end
task.save!
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the #up method looks good to me now


def down
conversion_host_ids = MiqRequestTask.where(:type => 'ServiceTemplateTransformationPlanTask').map do |task|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be scoped with in_my_region

return if task.conversion_host.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be next if

task.options[:transformation_host_id] = task.conversion_host.resource.id
agrare marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe task.conversion_host.resource_id to avoid loading resource (which I think would fail anyway)?

task.save!
task.conversion_host.id
agrare marked this conversation as resolved.
Show resolved Hide resolved
end.uniq.compact
ConversionHost.destroy(conversion_host_ids)

remove_column :miq_request_tasks, :conversion_host_id
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require_migration

describe AddConversionHostIdToMiqRequestTasks do
let(:task_stub) { migration_stub(:MiqRequestTask) }
let(:host_stub) { migration_stub(:Host) }
let(:conversion_host_stub) { migration_stub(:ConversionHost) }

migration_context :up do
agrare marked this conversation as resolved.
Show resolved Hide resolved
it "when host doesn't exist" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, but it would be great if this wrote out what we were testing for.

For example, it "doesn't set the conversion host when the host object doesn't exist"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

host = host_stub.create!
conversion_host_id = host.id
task = task_stub.create!(
:type => 'ServiceTemplateTransformationPlanTask',
:options => { :dummy_key => 'dummy_value', :transformation_host_id => conversion_host_id }
)
host.destroy!
agrare marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to generate a record id (in the current region) without creating and destroying a record

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how you could do that without querying the sequences. I think it would have been nice to create the host in a before block, but other than that I think this is fine.


migrate
This conversation was marked as resolved.
Show resolved Hide resolved
task.reload

expect(task.options).to eq(:dummy_key => 'dummy_value')
expect(AddConversionHostIdToMiqRequestTasks::ConversionHost.find_by(:resource_id => conversion_host_id)).to be_nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be conversion_host_stub.find_by... right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

expect(task.conversion_host).to be_nil
end

it "when host exist" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, it "references the existing Host in the ConversionHost record"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

host = host_stub.create!
task = task_stub.create!(
:type => 'ServiceTemplateTransformationPlanTask',
:options => { :dummy_key => 'dummy_value', :transformation_host_id => host.id }
)

migrate
task.reload

expect(task.options).to eq(:dummy_key => 'dummy_value')
expect(AddConversionHostIdToMiqRequestTasks::ConversionHost.find_by(:resource => host)).not_to be_nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I think this should use the stub.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

expect(task.conversion_host).to eq(AddConversionHostIdToMiqRequestTasks::ConversionHost.find_by(:resource => host))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

end
end

migration_context :down do
it "updates task.options" do
host = host_stub.create!
conversion_host = conversion_host_stub.create!(:resource => host)
task = task_stub.create!(
:type => 'ServiceTemplateTransformationPlanTask',
:conversion_host => conversion_host
)

migrate

task.reload
expect(task.options[:transformation_host_id]).to eq(host.id)
expect { conversion_host.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end