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

Update vm transformation status in a plan #17027

Merged
merged 2 commits into from
Feb 21, 2018

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented Feb 19, 2018

Each selected vm in a transformation plan has status column needs to be updated. This work enables the status update during the course of miq_request_task execution.

@bzwei
Copy link
Contributor Author

bzwei commented Feb 19, 2018

@miq-bot add_label transformation
@miq-bot add_label enhancement
@miq-bot assign @gmcculloug
@lfu @fdupont-redhat please review

@@ -201,9 +201,13 @@ def after_ae_delivery(ae_result)
def update_and_notify_parent(*args)
prev_state = state
super
return task_activated if state == "active" && prev_state != "active"
task_finished if state == "finished" && prev_state != "finished"
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about refactoring this to be more generic:

self.try("task_#{state}") if state != prev_state

The existing task_finished line on 205 can be deleted, task_activated would be renamed task_active and would not be required in the base model.


def vm_request
miq_request.vm_requests.find_by(:resource => source)
end
Copy link
Member

Choose a reason for hiding this comment

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

These should be private methods.


def task_finished
# update the status of vm transformation status in the plan
vm_request.update_attributes(:status => status == 'Ok' ? 'Completed' : 'Failed')
Copy link
Member

Choose a reason for hiding this comment

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

As each task finishes the request status will be updated based on the results of this single task? I'm not sure how this is going to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the miq_request, but service_resource wrapper for vm.

Copy link
Member

Choose a reason for hiding this comment

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

The name vm_request seems like it will be easily confused. I'm open to other ideas but maybe resource_status or vm_status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot be status., vm_transformation_demand or vm_resource?

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with vm_resource.

@miq-bot
Copy link
Member

miq-bot commented Feb 21, 2018

Checked commits bzwei/manageiq@1b885b6~...c50a2f0 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

@gmcculloug gmcculloug merged commit 68544e0 into ManageIQ:master Feb 21, 2018
@gmcculloug gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 21, 2018
@simaishi
Copy link
Contributor

@gmcculloug Shouldn't this be gaprindashvili/yes? (Noticed because cherry-picking #17256 resulted in conflicts)

@simaishi
Copy link
Contributor

@gmcculloug ^ ping

@gmcculloug
Copy link
Member

Yes, I'll add the label.

simaishi pushed a commit that referenced this pull request May 30, 2018
Update vm transformation status in a plan
(cherry picked from commit 68544e0)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 0ee713ac66ddd55242ad2bd6629288b31f886d84
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Feb 21 17:40:10 2018 -0500

    Merge pull request #17027 from bzwei/vm_migration_status
    
    Update vm transformation status in a plan
    (cherry picked from commit 68544e01fc9fb9c5867b13a8198fb7b4ca0f1ab5)

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