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

Refactoring cloud vm retirement pre_retirement method #259

Merged
merged 2 commits into from
Jun 5, 2018
Merged

Refactoring cloud vm retirement pre_retirement method #259

merged 2 commits into from
Jun 5, 2018

Conversation

pkomanek
Copy link
Contributor

@pkomanek pkomanek commented Mar 7, 2018

Purpose or Intent

Refactoring Cloud/VM/Retirement/StateMachines/Methods.class/methods/pre_retirement.rb method with spec. This PR is based on the issue bellow.

Links

#8

@miq-bot add_label refactoring

def main
vm = @handle.root['vm']
if vm && vm.power_state == 'on'
ems = vm.ext_management_system
Copy link
Contributor

Choose a reason for hiding this comment

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

@tinaafitz if the ems is missing and the vm is powered on, should we be raising an exception and stopping the workflow or should we keep continuing. I remember we had some corner cases that we were trying to solve where a failed retirement is restarted. If there is no ems should we log the fact that its an orphaned vm.

Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor We should continue processing if the ems is nil and the VM will have the power state of "unknown" in that case

@mkanoor
Copy link
Contributor

mkanoor commented Mar 7, 2018

@pkomanek
When you run the specs can you check the code coverage for the method
from the shell set an environment variable CI, run the spec and check coverage for the file

export CI=1
bundle exec rspec <spec_file_name>
In the current directory check for a file under coverage/index.html and see if all lines have been tested. 

end

it 'does not stop a vm in \'powered_off\' state' do
vm.update_attribute(:raw_power_state, "PowerOff")
Copy link
Member

Choose a reason for hiding this comment

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

Use: vm.update_attributes(:raw_power_state => "PowerOff")

def main
vm = @handle.root['vm']
if vm && vm.power_state == 'on'
ems = vm.ext_management_system
Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor We should continue processing if the ems is nil and the VM will have the power state of "unknown" in that case

vm = @handle.root['vm']
if vm && vm.power_state == 'on'
ems = vm.ext_management_system
@handle.log('info', "Stopping Instance <#{vm.name}> in EMS <#{ems.try(:name)}>")
Copy link
Member

Choose a reason for hiding this comment

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

@pkomanek Can you issue the log message only if there is an ems? You can remove the try from the log message in that case.
something like
if ems
@handle.log('info', "Stopping Instance <#{vm.name}> in EMS <#{ems.name}>")
vm.stop
end

@miq-bot
Copy link
Member

miq-bot commented Mar 14, 2018

Checked commits pkomanek/manageiq-content@04ce09a~...cc615ec with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@pkomanek pkomanek changed the title [WIP]Refactoring cloud vm retirement pre_retirement method Refactoring cloud vm retirement pre_retirement method Mar 14, 2018
@miq-bot miq-bot removed the wip label Mar 14, 2018
@tinaafitz
Copy link
Member

bump @mkanoor @gmcculloug

@mkanoor mkanoor merged commit a91443d into ManageIQ:master Jun 5, 2018
@mkanoor mkanoor added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 5, 2018
@pkomanek pkomanek deleted the refactoring_cloud_vm_retirement_statemachines_methods_PreRetirement_method branch June 5, 2018 13:57
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.

5 participants