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

OC-9368 Image_os_type option should be compulsory in knife-openstack. #48

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/chef/knife/cloud/openstack_server_create_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ def self.included(includer)
:default => ["default"],
:proc => Proc.new { |groups| groups.split(',') }

option :ssh_key_name,
option :openstack_ssh_key_id,
:short => "-S KEY",
:long => "--ssh-key KEY",
:long => "--openstack-ssh-key-id KEY",
:description => "The OpenStack SSH keypair id",
:proc => Proc.new { |key| Chef::Config[:knife][:openstack_ssh_key_id] = key }

Expand Down
6 changes: 0 additions & 6 deletions lib/chef/knife/openstack_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ def primary_public_ip_address(addresses)
return addresses['public'].last['addr'] if addresses['public']
end

def is_image_windows?
# Openstack image info does not have os type, so we use image_os_type cli option to interpret if we are creating windows server.
os_type = locate_config_value(:image_os)
os_type.nil? ? false : (os_type.downcase == 'windows')
end

def create_service_instance
Copy link

Choose a reason for hiding this comment

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

There should be a method set_image_os_type in knife-cloud with the default implementation. The method should set the field image_os_type.
In plugins with different behavior, this method can be overridden.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks!

OpenstackService.new
end
Expand Down
5 changes: 4 additions & 1 deletion lib/chef/knife/openstack_server_create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def validate_params!
if locate_config_value(:identity_file).nil? && locate_config_value(:ssh_password).nil?
errors << "You must provide either Identity file or SSH Password."
end
if !locate_config_value(:identity_file).nil? && (config[:ssh_key_name].nil? && Chef::Config[:knife][:openstack_ssh_key_id].nil?)
if !locate_config_value(:identity_file).nil? && locate_config_value(:openstack_ssh_key_id).nil?
errors << "You must provide SSH Key."
end
elsif locate_config_value(:bootstrap_protocol) == 'winrm'
Expand All @@ -115,6 +115,9 @@ def validate_params!
else
errors << "You must provide a valid bootstrap protocol. options [ssh/winrm]. For linux type images, options [ssh]"
end

errors << "You must provide --image-os-type option [windows/linux]" if ! (%w(windows linux).include?(locate_config_value(:image_os_type)))
Copy link

Choose a reason for hiding this comment

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

This check should go in knife-cloud.
The image_os_type field is compulsory for all - but it needs to be entered explicitly by the user of openstack.
For other plugins, we must set it programatically.


exit 1 if errors.each{|e| ui.error(e)}.any?
end
end
Expand Down
21 changes: 19 additions & 2 deletions spec/unit/openstack_server_create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,32 @@
end

it "validates ssh params" do
Chef::Config[:knife][:image_os] = "other"
Chef::Config[:knife][:image_os_type] = "linux"
Chef::Config[:knife][:bootstrap_protocol] = "ssh"
instance.ui.should_receive(:error).with("You must provide either Identity file or SSH Password.")
instance.validate_params!
end

it "raise_error on invalid image_os_type params" do
Chef::Config[:knife][:ssh_password] = "ssh_password"
Chef::Config[:knife][:openstack_ssh_key_id] = "ssh_key"
Chef::Config[:knife][:image_os_type] = "other_than_windows_linux"
instance.ui.should_receive(:error).with("You must provide --image-os-type option [windows/linux]")
instance.validate_params!
end

it "raise_error on mising image_os_type params" do
Chef::Config[:knife][:image_os_type] = "other_than_windows_linux"
Chef::Config[:knife][:ssh_password] = "ssh_password"
Chef::Config[:knife][:openstack_ssh_key_id] = "ssh_key"
instance.ui.should_receive(:error).with("You must provide --image-os-type option [windows/linux]")
instance.validate_params!
end

context "bootstrap protocol: Ssh " do
before(:each) do
Chef::Config[:knife][:bootstrap_protocol] = "ssh"
Chef::Config[:knife][:image_os_type] = 'linux'
end

it "raise error when neither identity file nor SSH password is provided and exits immediately." do
Expand Down Expand Up @@ -96,7 +113,7 @@
before(:each) do
instance.configure_chef
instance.config[:bootstrap_protocol] = 'winrm'
Chef::Config[:knife][:image_os] = 'windows'
Chef::Config[:knife][:image_os_type] = 'windows'
end

it "validates gracefully when winrm User and Winrm password are provided." do
Expand Down