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

Refactoring cloud vm provisioning placement best fit methods #353

Merged
merged 4 commits into from
Nov 2, 2018
Merged

Refactoring cloud vm provisioning placement best fit methods #353

merged 4 commits into from
Nov 2, 2018

Conversation

pkomanek
Copy link
Contributor

Purpose or Intent

  1. Refactoring Cloud/VM/Provisioning/Placement.class/__methods__/best_fit_*.rb methods and adding the specs for these methods.
  2. Adding the _spec suffix into the best_fit_amazon spec name.
  3. Adding a missing test into the best_fit_amazon_spec.rb file.

This PR is based on the issue bellow.

Links

Issue: #8

@miq-bot add_label refactoring


if prov.get_option(:cloud_network).nil?
cloud_network = prov.eligible_cloud_networks.first
set_cloud_network if @prov.get_option(:cloud_network).nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek would this work

 prov.get_option(:cloud_network) || set_cloud_network
 prov.get_option(:cloud_subnet) || set_cloud_subnet
 prov.get_option(:resource_group) || set_resource_group

If the user has not provided values we are providing a default maybe the method names could also be changed to default_cloud_network, default_cloud_subnet, default_resource_group where we pick the first element from an array of choices.

end
end

if $PROGRAM_NAME == __FILE__
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek
Based on this PR #390 we dont need the if block anymore you only need line number 65 by itself and the spec_helper will mock this class at load time

end
end

if $PROGRAM_NAME == __FILE__
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek only need line number 53

end
end

if $PROGRAM_NAME == __FILE__
ManageIQ::Automate::Cloud::VM::Provisioning::Placement::BestFitOpenStack.new.main
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek Only need line number 44

$evm.log("info", "Selected Cloud Network: #{cloud_network.name}")
end
end
private
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek
can we check if prov is present and raise an error, also prov could just be a method

something similar to

def prov
   @prov ||= @handle.root["miq_provision"].tap do |req|
     raise "miq_provision not provided"
  end
end

end

def main
@prov = @handle.root["miq_provision"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek
I think we should by using a prov method which can check for missing field raise an error similar to what you have in the other file in this PR

image = prov.vm_template
raise "Image not specified" if image.nil?
def main
@prov = @handle.root["miq_provision"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek Use prov method

@miq-bot
Copy link
Member

miq-bot commented Nov 2, 2018

Checked commits pkomanek/manageiq-content@7e7c7b8~...a86d5c2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 🍪

@mkanoor mkanoor merged commit cccf64e into ManageIQ:master Nov 2, 2018
@mkanoor mkanoor added this to the Sprint 98 Ending Nov 5, 2018 milestone Nov 2, 2018
@pkomanek pkomanek deleted the refactoring_cloud_vm_provisioning_placement_best_fit_methods branch November 9, 2018 10:29
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.

3 participants