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

Generic Service State Machine update_status change #85

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

tinaafitz
Copy link
Member

@tinaafitz tinaafitz commented Mar 31, 2017

Modified the update_status method to use new on_error method.

https://www.pivotaltracker.com/story/show/142796573

Related PR ManageIQ/manageiq#14583

@tinaafitz
Copy link
Member Author

@miq-bot add-label services, fine/yes

@gmcculloug
Copy link
Member

@bzwei Please review

end

context "on_error provisioning" do
let(:root_object) do
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 move let(:root_object) out of the context block, and assign 'service_action' => service_action. Then within each context block you only need to have let(:service_action) { 'Provision/Retirement'}


def updated_message(status)
updated_message = "Server [#{@handle.root['miq_server'].name}] "
updated_message += "Service [#{service.name}] #{service_action} "
Copy link
Contributor

Choose a reason for hiding this comment

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

@tinaafitz the service method is being called here to fetch the service object, if someone wants to log another property from the service object, they would call service.some_property that would end up calling the method again. So can we pass the service into this method

# Get status from input field status
status = @handle.inputs['status']

update_task(updated_message(service, status), status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both lines have update_message(service, status). Is it right?

# Get status from input field status
status = @handle.inputs['status']

update_task(updated_message(service, status), status)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tinaafitz
We would be calling the service function multiple times here, in case of error there are 3 method calls.
Maybe we can do this in a separate PR. We can either use a @service attribute in the method and just use it everywhere, so we dont have to pass it around or call the function multiple times

@miq-bot
Copy link
Member

miq-bot commented Mar 31, 2017

Checked commit tinaafitz@3263b0e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

@mkanoor
Copy link
Contributor

mkanoor commented Apr 3, 2017

👍

@mkanoor mkanoor merged commit c9739f2 into ManageIQ:master Apr 3, 2017
@mkanoor mkanoor added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 3, 2017
simaishi pushed a commit that referenced this pull request Apr 3, 2017
Generic Service State Machine update_status change
(cherry picked from commit c9739f2)
@simaishi
Copy link
Contributor

simaishi commented Apr 3, 2017

Fine backport details:

$ git log -1
commit bfe87c5d55f1c98077b69a3c0f5ea8f859b6095c
Author: Madhu Kanoor <mkanoor@redhat.com>
Date:   Mon Apr 3 10:04:08 2017 -0400

    Merge pull request #85 from tinaafitz/generic_service_on_error
    
    Generic Service State Machine update_status change
    (cherry picked from commit c9739f262ab436bc0142eeab978f929a681b7400)

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.

6 participants