-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix default cleanup state machine #382
Fix default cleanup state machine #382
Conversation
4c41360
to
baa34b4
Compare
Checked commit fabiendupont@baa34b4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 content/automate/ManageIQ/Transformation/Infrastructure/VM/Common.class/methods/checkpoweredon.rb
|
else | ||
task = @handle.root['service_template_transformation_plan_task'] | ||
vm = @handle.vmdb(:vm).find_by(:id => task.get_option(:destination_vm_id)) if task.present? | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should break this down into methods which would make the logic easier to read.
Move this first if-else block into a method that returns both task
and vm
:
def task_and_vm
if @handle.root['service_template_transformation_plan_task'].blank?
task = @handle.vmdb(:service_template_transformation_plan_task).find_by(:id => @handle.root['service_template_transformation_plan_task_id'])
vm = task.source if task.present?
else
task = @handle.root['service_template_transformation_plan_task']
vm = @handle.vmdb(:vm).find_by(:id => task.get_option(:destination_vm_id)) if task.present?
end
return task, vm
end
Then the first part of main becomes:
task, vm = task_and_vm
If you think this is too much change it could be in a followup refactoring, but we need to move away for having large single method scripts in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is identical in checkpoweredon.rb
and poweron.rb
. I'd propose moving task_and_vm
to a embedded utility method. It's a bigger change that may require another PR. I'd go for a followup refactoring.
return if vm.blank? | ||
@handle.log(:info, "Target VM: #{vm.name} [#{vm.vendor}]") | ||
return if task.get_option(:source_vm_power_state) != 'on' | ||
if vm.power_state != 'on' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability add spaces between lines when the context changes:
I would think something like this:
def main
task, vm = task_and_vm
return if vm.blank?
@handle.log(:info, "Target VM: #{vm.name} [#{vm.vendor}]")
return if task.get_option(:source_vm_power_state) != 'on'
if vm.power_state != 'on'
@handle.root["ae_result"] = "retry"
@handle.root["ae_retry_interval"] = "15.seconds"
end
rescue => e
@handle.set_state_var(:ae_state_progress, 'message' => e.message)
raise
end
…hine_poweron Fix default cleanup state machine (cherry picked from commit 3e6a3f6) https://bugzilla.redhat.com/show_bug.cgi?id=1608758
Gaprindashvili backport details:
|
While looking to refactor
CheckPoweredOn
state, I realized that the cleanup state machine was targeting the destination VM for power on, while it should target source VM. The logic is similar to what is implemented inpoweron.rb
in #378. This PR implements it incheckpoweredon.rb
.Doing so makes the
checkpoweredon.rb
method generic, so it's moved to/Transformation/Infrastructure/VM/Common
class. This requires changing two state machines:PostTransform
andTransformationCleanup
.Changing
TransformationCleanup
state machine removes the need fordestination_ems_type
state var. The methodassesstransformationcleanup.rb
only purpose was to set this state var, so it can be removed.When running tests, I also realized that
service_template_transformation_plan_task_id
attribute was missing in$evm.root
. Somehow, the change isweightedupdatestatus.rb
to add it to the cleanup request was not pushed in #378, so I'm fixing it here.Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1599997