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

[WIP] Add v2v transformation validation. #17251

Closed
wants to merge 1 commit into from
Closed

Conversation

lfu
Copy link
Member

@lfu lfu commented Apr 4, 2018

Follow up of #17177.

@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, transformation

cc @bzwei

@@ -1,9 +1,53 @@
class TransformationMapping < ApplicationRecord
VM_VALID = N_("OK").freeze
Copy link
Member

Choose a reason for hiding this comment

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

You do not want to set this as a constant. It will get set once with whatever localization is in place when the model loads. You are trying to honor the current users local which is dynamic.

app/models/vm.rb Outdated
@@ -121,6 +121,12 @@ def supported_consoles
}
end

def v2v_migrated?
ServiceResource.where(:resource => self, :status => 'complete').any? do |resource|
Copy link
Member

Choose a reason for hiding this comment

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

@bzwei Is complete a constant somewhere in the transformation models that we can reference?

I actually think this is should be "Completed" from here: app/models/service_template_transformation_plan_task.rb#L30. Which feels like it should really be a constant in the model.

def select_vms
valid_list = []

Vm.each do |vm|
Copy link
Member

Choose a reason for hiding this comment

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

While the .each method does not exist on the Vm model is this really what we want to do? It will return every infra and cloud VM object in the database. It should at least be limited to the source provider defined in the mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can aggregate all source clusters from the mapping and find all VMs under the clusters.

def select_vms
valid_list = []

Vm.each do |vm|
Copy link
Contributor

Choose a reason for hiding this comment

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

You can aggregate all source clusters from the mapping and find all VMs under the clusters.

app/models/vm.rb Outdated
@@ -121,6 +121,12 @@ def supported_consoles
}
end

def v2v_migrated?
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of the method is to identify whether the VM can be added to a plan. A valid VM is the one has ServiceResource#status as Failed or never exists in ServiceResource.

Also reconsider the method name.

app/models/vm.rb Outdated
@@ -121,6 +121,12 @@ def supported_consoles
}
end

def v2v_migrated?
ServiceResource.where(:resource => self, :status => 'complete').any? do |resource|
ServiceTemplate.find(resource.service_template_id).kind_of?(ServiceTemplateTransformationPlan)
Copy link
Contributor

Choose a reason for hiding this comment

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

why another query? Can be resource.service_template.kind_of?(ServiceTemplateTransformationPlan).

@miq-bot
Copy link
Member

miq-bot commented Apr 5, 2018

Checked commit lfu@0ca2d4a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@lfu lfu closed this Apr 6, 2018
@lfu
Copy link
Member Author

lfu commented Apr 6, 2018

Combined with #17177.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants