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.

Sign up for GitHub

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

Jump to bottom

Support for v2v pre/post Ansible playbook service. #17627

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Support for v2v pre/post Ansible playbook service. #17627

merged 1 commit into from
Jul 5, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Jun 22, 2018 •
edited by gmcculloug
Loading

@gmcculloug gmcculloug requested a review from bzwei June 22, 2018 21:23
@bzwei
Copy link
Contributor

bzwei commented Jun 25, 2018

@lfu Looks good. You have more coming up, right?

@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 49486b1 to 48aaeed Compare June 25, 2018 17:47
@lfu lfu changed the title [WIP] Support for v2v pre/post Ansible playbook service. Support for v2v pre/post Ansible playbook service. Jun 25, 2018
@lfu
Copy link
Member Author

lfu commented Jun 25, 2018

@bzwei Ready for final review.

@miq-bot miq-bot removed the wip label Jun 25, 2018
bzwei
bzwei reviewed Jun 25, 2018
View reviewed changes
app/models/service_template_transformation_plan_task.rb
@@ -20,6 +20,14 @@ def transformation_destination(source_obj)
miq_request.transformation_mapping.destination(source_obj)
end

def pre_ansible_playbook_service_template
Copy link
Contributor

Choose a reason for hiding this comment

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

need to expose the methods in service model.

bzwei
bzwei reviewed Jun 25, 2018
View reviewed changes
spec/models/service_template_transformation_plan_task_spec.rb
@@ -52,6 +55,14 @@
it { expect(task.transformation_destination(src)).to eq(dst) }
end

describe '#pre_ansible_playbook_service_template' do
Copy link
Contributor

Choose a reason for hiding this comment

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

please add negative test where pre/post_service_id are not configured.

@bzwei
Copy link
Contributor

bzwei commented Jun 25, 2018

cc @fdupont-redhat

@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 48aaeed to 52ab78d Compare June 25, 2018 20:28
@lfu lfu mentioned this pull request Jun 25, 2018
Merged
@lfu
Copy link
Member Author

lfu commented Jun 27, 2018

@bzwei Updated.

@michaelkro michaelkro mentioned this pull request Jun 28, 2018
Merged
8 tasks
bzwei
bzwei reviewed Jun 28, 2018
View reviewed changes
app/models/service_template_transformation_plan.rb Outdated
service_template.create_resource_actions(enhanced_config_info)
end
end
end

def add_vm_resource(vm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this logic into #validate_config_info

@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 52ab78d to 163376f Compare June 28, 2018 18:23
gmcculloug
gmcculloug reviewed Jul 3, 2018
View reviewed changes
app/models/service_template_transformation_plan.rb Outdated
# :vm_ids
# :pre_service_id
# :post_service_id
# :tasks => [
Copy link
Member

Choose a reason for hiding this comment

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

We are overloading the term tasks here since the migration is built on request/request_task modeling. I suggest renaming this to actions.

gmcculloug
gmcculloug reviewed Jul 3, 2018
View reviewed changes
app/models/service_template_transformation_plan.rb Outdated
config_info[:vms]
end
pre_service_id = config_info[:pre_service] ? config_info[:pre_service].id : config_info[:pre_service_id]
post_service_id = config_info[:post_service] ? config_info[:post_service_id].id : config_info[:post_service_id]
Copy link
Member

Choose a reason for hiding this comment

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

When do we have the pre_service object and when do we only get passed the pre_service_id?

Could these lines be simplified to this format:
pre_service_id = config_info[:pre_service].try(:id) || config_info[:pre_service_id]

gmcculloug
gmcculloug reviewed Jul 3, 2018
View reviewed changes
app/models/service_template_transformation_plan.rb Outdated
next if vm_obj.nil?

vm_options = {}
vm_options[:pre_ansible_playbook_service_template_id] = pre_service_id if vm_hash[:pre_service]
Copy link
Member

Choose a reason for hiding this comment

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

@bzwei Instead of passing the service template id down into each task would it make sense to just capture the :pre_service and :post_service flags for each and at runtime do the lookup from the associated plan.
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only store an id, it makes no big difference to use id vs boolean, but it saves extra logic to resolve the id at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about having to keep the id in-sync across all the records if someone edited the plan and selected a new service. You would not need to adjust each task and the value is really only looked up once during the run so the overhead at that time is minimal.

@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 163376f to d987c56 Compare July 3, 2018 14:46
bzwei
bzwei reviewed Jul 3, 2018
View reviewed changes
app/models/service_template_transformation_plan.rb Outdated

vms = []
if config_info[:actions]
vm_objects = VmOrTemplate.where(:id => config_info[:actions].collect { |vm_hash| vm_hash[:vm_id] }.compact)
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 make vm_objects a hash rather than an array. Then in Ln 89 you can directly lookup the object without a looping.
Ln 88 could be as simple as vm_obj = vm_objects[vm_hash[:vm_id]] || vm_hash[:vm]

@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from d987c56 to 11a51c0 Compare July 3, 2018 15:38
@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 11a51c0 to 64d12b1 Compare July 3, 2018 16:43
@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2018

Checked commit lfu@64d12b1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

@bzwei
Copy link
Contributor

bzwei commented Jul 5, 2018

LGTM

@gmcculloug gmcculloug merged commit ce4d2b5 into ManageIQ:master Jul 5, 2018
@gmcculloug gmcculloug added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 5, 2018
@AparnaKarve AparnaKarve mentioned this pull request Jul 6, 2018
Closed
@AparnaKarve
Copy link
Contributor

@lfu @bzwei The changes here seem to be causing an API failure for service_templates during Migration Plan creation. Can you please take a look? Thanks.

ManageIQ/manageiq-v2v#471

@lfu lfu mentioned this pull request Jul 6, 2018
Merged
@AparnaKarve
Copy link
Contributor

@miq-bot add_label gaprindashvili/yes

@AparnaKarve AparnaKarve mentioned this pull request Jul 11, 2018
Merged
simaishi pushed a commit that referenced this pull request Jul 25, 2018
@gmcculloug @simaishi
Merge pull request #17627 from lfu/v2v_pre_post_ansibile_service
d45d101 
Support for v2v pre/post Ansible playbook service.
(cherry picked from commit ce4d2b5)

https://bugzilla.redhat.com/show_bug.cgi?id=1608420
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit d45d1019b6ff0647c0919a2affc6a22ef3819684
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Jul 5 09:54:50 2018 -0400

    Merge pull request #17627 from lfu/v2v_pre_post_ansibile_service
    
    Support for v2v pre/post Ansible playbook service.
    (cherry picked from commit ce4d2b5ac92795a615cb7ae98b211bbac205ee57)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1608420

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzwei bzwei bzwei approved these changes

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@gmcculloug gmcculloug

Labels
blocker enhancement gaprindashvili/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lfu @bzwei @miq-bot @AparnaKarve @simaishi @gmcculloug