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

Decide default destination folder for cloning #7150

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented Mar 8, 2016

@@ -47,10 +47,6 @@ def prepare_for_clone_task
cluster ? cluster.default_resource_pool : dest_host.default_resource_pool
end
Copy link
Member

Choose a reason for hiding this comment

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

@bzwei In a separate PR, can we create a similar dest_resource_pool method and move all this logic there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chessbyte yes sir 👌

@bzwei bzwei force-pushed the vmware_placement branch 2 times, most recently from 5291de5 to 873a714 Compare March 8, 2016 21:13

host_dc = dest_host.parent_datacenter || dest_host.ems_cluster.parent_datacenter
proposed_folder = "Datacenters/#{host_dc.name}/vm/Discovered virtual machine"
EmsFolder.where(:name => 'Discovered virtual machine').detect do |f|
Copy link
Member

Choose a reason for hiding this comment

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

@bzwei VMware provisioning requires that a folder be passed so my concern is that the "Discovered virtual machine" folder may not exist for some reason. In that case we should drop back to use the root vm folder.

I think the lookup logic should be move into a new method that we can call with each path and it would return the folder or nil.

Something like:
vm_folder = "Datacenters/#{host_dc.name}/vm"
find_dest_folder("#{vm_folder}/Discovered virtual machine") || find_dest_folder(vm_folder)

@gmcculloug
Copy link
Member

@bzwei This PR should also include the removal of the set_folder logic in
vmware_best_fit_least_utilized.rb#L8

@bzwei
Copy link
Contributor Author

bzwei commented Mar 9, 2016

@gmcculloug I removed set_folder in the best_fit code.

@gmcculloug
Copy link
Member

@bzwei The failing tests are in a context ("Auto placement #set_folder") that is no longer needed since the method is not setting the folder. The let(:vm_folder) is no longer needed in that method as well.

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2016

Checked commit bzwei@c755248 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
98 files checked, 5 offenses detected

spec/automation/unit/method_validation/vmware_best_fit_least_utilized_spec.rb

gmcculloug added a commit that referenced this pull request Mar 16, 2016
Decide default destination folder for cloning
@gmcculloug gmcculloug merged commit 1c7773a into ManageIQ:master Mar 16, 2016
@gmcculloug gmcculloug added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 16, 2016
@bzwei bzwei deleted the vmware_placement branch March 16, 2016 15:14
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