-
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 Service Retirement requests leaving the Service as 'retiring'. #530
Conversation
Pull Request Test Coverage Report for Build 3018
💛 - Coveralls |
Pull Request Test Coverage Report for Build 3132
💛 - Coveralls |
403306f
to
cb681f9
Compare
Start_retirement is an old style method with no spec. I don't think it makes sense to build an old style spec for the old method. I'm going to followup with a PR to refactor the method and add a spec test. |
$evm.log(:error, "The old style retirement is incompatible with the new retirement state machine.") | ||
exit(MIQ_ABORT) | ||
end | ||
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.
Should this be:
unless $evm.root['service_retire_task']
$evm.log(:error, "Service retire task not found")
$evm.log(:error, "The old style retirement is incompatible with the new retirement state machine.")
exit(MIQ_ABORT)
end
This check should be placed before $evm.create_notification(:type => :service_retiring, :subject => service)
cb681f9
to
9e1f8b1
Compare
@lfu Made changes as you requested, please review. |
@@ -20,6 +20,14 @@ | |||
exit MIQ_ABORT | |||
end | |||
|
|||
$evm.root['service_retire_task'].tap do |task| |
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.
Is .tap
necessary here?
9e1f8b1
to
5bacade
Compare
…tiring'. This will fix the problem when someone uses the older api call and leaves the service in a 'retiring' state. If there is no task, the process is aborted and the state is 'initializing'. This will allow the retirement process will start again instead of being denied because the state is 'retiring' fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1698480 Updated update_service_retirement_status_spec Moved check for service_retire_task before notification as requested. Removed tap and changed code as requested Changed 'fred' to 'active' as requested
5bacade
to
8b95631
Compare
Checked commit billfitzgerald0120@8b95631 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
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.
thanks!
@billfitzgerald0120 |
@miq-bot add_label hammer/yes |
Fix Service Retirement requests leaving the Service as 'retiring'. (cherry picked from commit ac0a57a) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1713477
Hammer backport details:
|
This will fix the problem when someone uses the older api call and leaves the service in a 'retiring' state.
If there is no task, the process is aborted and the state is 'initializing'.
This will allow the retirement process will start again instead of being denied because the state is 'retiring'
fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1698480
@miq-bot add_label bug
@miq-bot assign @tinaafitz
Start_retirement is an old style method with no spec. I don't think it makes sense to build an old style spec for the old method. I would like to followup this PR with a PR that refactors the method and I will build a spec for that.