Skip to content
This repository has been archived by the owner on Jul 29, 2018. It is now read-only.

Fix #327 and #326 Tests and docs for --cli-version and --path options #414

Merged
merged 1 commit into from
Oct 18, 2016
Merged

Fix #327 and #326 Tests and docs for --cli-version and --path options #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,6 @@ Metrics/AbcSize:
Exclude:
- 'lib/vagrant-service-manager/command.rb'
- 'Rakefile'

Lint/Eval:
Enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this might be needed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @hferentschik

14 changes: 10 additions & 4 deletions commands.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,16 @@ Similarly, you can use `$ vagrant service-manager status openshift`,

If you need to install the client binary for a specified service, use: +

`vagrant service-manager install-cli [service]` +
`vagrant service-manager install-cli [service] [options]` +
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


The possible options for service are `docker` and `openshift`. +

The following options can be used: +

`--cli-version value`:: Specify the version of the binary as value to be installed +
`--path value`:: Specify the absolute or relative path as value where the binary should be installed +
`-h, --help`:: Prints help for the specific command being run.

As per default, binaries are downloaded to
`$VAGRANT_HOME/data/service-manager/bin/<service>/<cli-version>`, where
`$VAGRANT_HOME` defaults to `.vagrant.d` in your home directory.
Expand All @@ -236,11 +242,11 @@ As per default, binaries are downloaded to
To install client binary for Docker:

----------------------------------------------------------------------------------------------------
$ vagrant service-manager install-cli docker
# Binary already available at /home/johndoe/.vagrant.d/data/service-manager/bin/docker/1.9.1/docker
$ vagrant service-manager install-cli docker --cli-version 1.9.1 --path /home/johndoe/bin/docker
# Binary already available at /home/johndoe/bin/docker
# run binary as:
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

# docker <command>
export PATH=/home/johndoe/.vagrant.d/data/service-manager/bin/docker/1.9.1:$PATH
export PATH=/home/johndoe/bin:$PATH

# run following command to configure your shell:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli docker | tr -d '\r')"
Expand Down
8 changes: 8 additions & 0 deletions features/adb-openshift.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ Feature: Command output from various OpenShift related commands in ADB
Then the exit status should be 0
And the binary "oc" should be installed

When I run `bundle exec vagrant service-manager install-cli openshift --cli-version 1.3.0`
Then the exit status should be 0
And the binary "oc" of service "openshift" should be installed with version "1.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

I created issue #415 to follow up on what's going on with CI and why we did not get test failures when some of the tests were clearly wrong. We need to get to the bottom of this, otherwise we cannot trust CI results, in particular since these tests are not run per default on the dev machine


When I evaluate and run `bundle exec vagrant service-manager install-cli openshift --path #{ENV['VAGRANT_HOME']}/oc`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I like how the tests look like now. Also nice to use the same path in the When and Then clause and that it gets transparently interpolated.

Then the exit status should be 0
And the binary should be installed in path "#{ENV['VAGRANT_HOME']}/oc"

When I successfully run `bundle exec vagrant reload`
And I successfully run `bundle exec vagrant service-manager status openshift`
Then the exit status should be 0
Expand Down
13 changes: 13 additions & 0 deletions features/cdk-openshift.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ Feature: Command output from various OpenShift related commands in CDK
Then the exit status should be 0
And the binary "oc" should be installed

When I run `bundle exec vagrant service-manager install-cli openshift --cli-version 1.3.0`
Then the exit status should be 0
And the binary "oc" of service "openshift" should be installed with version "1.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I think this is better


When I evaluate and run `bundle exec vagrant service-manager install-cli openshift --path #{ENV['VAGRANT_HOME']}/oc`
Then the exit status should be 0
And the binary should be installed in path "#{ENV['VAGRANT_HOME']}/oc"

When I successfully run `bundle exec vagrant reload`
And I successfully run `bundle exec vagrant service-manager status openshift`
Then the exit status should be 0
And the service "openshift" should be running

Examples:
| box | provider | ip |
| cdk | virtualbox | 10.10.10.42 |
Expand Down
12 changes: 11 additions & 1 deletion features/install-cli.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ Feature: Command behavior of client side tools installation
A supported service. For example: docker, kubernetes or openshift.

Options:
-h, --help print this help
-h, --help print this help
--cli-version binary version to install
--path absolute or relative path where to install the binary

Example:
vagrant service-manager install-cli docker
Expand All @@ -52,6 +54,14 @@ Feature: Command behavior of client side tools installation
Then the exit status should be 0
And the binary "docker" should be installed

When I run `bundle exec vagrant service-manager install-cli docker --cli-version 1.12.1`
Then the exit status should be 0
And the binary "docker" of service "docker" should be installed with version "1.12.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this step is also quite nice now. Allows us to do a dedicated check in its implementation


When I evaluate and run `bundle exec vagrant service-manager install-cli docker --path #{ENV['VAGRANT_HOME']}/docker`
Then the exit status should be 0
And the binary should be installed in path "#{ENV['VAGRANT_HOME']}/docker"

Examples:
| box | provider | ip |
| adb | virtualbox | 10.10.10.42 |
Expand Down
22 changes: 21 additions & 1 deletion features/support/env.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,5 +150,25 @@ def extract_process_id(data)
end

expect(binaries.size).to eq(1)
expect(File.executable?(binaries[0])).to eq(true)
expect(File.executable?(binaries.first)).to eq(true)
end

Then(/^the binary "([^"]*)" of service "([^"]*)" should be installed with version "([^"]*)"$/) do |b, s, v|
binary_path = "#{ENV['VAGRANT_HOME']}/data/service-manager/bin/#{s}/#{v}/#{b}"
expect(File.file?(binary_path)).to eq(true)
expect(File.executable?(binary_path)).to eq(true)
end

Then(/^the binary should be installed in path "([^"]*)"$/) do |path|
binary_path = eval('"' + path + '"')
expect(File.file?(binary_path)).to eq(true)
expect(File.executable?(binary_path)).to eq(true)
end

# Per default the built-in Aruba 'When I run' will not evaluate/interpolate the string.
# We create a custom step definition to work around this
When(/^I evaluate and run `([^`]*)`$/) do |cmd|
cmd = eval('"' + cmd + '"')
cmd = sanitize_text(cmd)
run_simple(cmd, fail_on_error: false)
end
3 changes: 2 additions & 1 deletion lib/vagrant-service-manager/command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,8 @@ def execute_install_cli(service, options = {})
end

with_target_vms(nil, single_target: true) do |machine|
args = [service.to_sym, machine, @env, options]
options[:type] = service.to_sym
args = [machine, @env, options]

begin
args.last[:box_version] = machine.guest.capability(:os_variant)
Expand Down
19 changes: 13 additions & 6 deletions lib/vagrant-service-manager/installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
module VagrantPlugins
module ServiceManager
class Installer
def initialize(type, machine, env, options)
@type = type
def initialize(machine, env, options)
@options = options
@type = @options[:type]
@machine = machine
@env = env
@box_version = options[:box_version]

validate_prerequisites
binary_handler_class = Object.const_get(handler_class)
@binary_handler = binary_handler_class.new(machine, env, { type: @type }.merge(options))
@binary_handler = binary_handler_class.new(machine, env, @options)
end

def handler_class
"#{ServiceManager.name}::#{@box_version.upcase}#{@type.capitalize}BinaryHandler"
"#{ServiceManager.name}::#{@options[:box_version].upcase}#{@type.capitalize}BinaryHandler"
end

def install
Expand All @@ -30,7 +30,14 @@ def install

def validate_prerequisites
return if PluginUtil.service_running?(@machine, @type.to_s)
@env.ui.info I18n.t('servicemanager.commands.install_cli.service_not_enabled', service: @type)
@env.ui.info I18n.t('servicemanager.commands.install_cli.service_not_enabled',
service: @type)
exit 126

# return if --path is specified and directory mentioned by it exists
return if @options.key?('--path') && File.exist?(File.dirname(@options['--path']))
@env.ui.info I18n.t('servicemanager.commands.install_cli.invalid_binary_path',
dir_path: File.dirname(@options['--path']))
exit 126
end
end
Expand Down
4 changes: 3 additions & 1 deletion lib/vagrant-service-manager/plugin_util.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,11 @@ def self.fetch_existing_oc_binary_path_in_windows
path_string = ENV['PATH']

separator = ':' unless path_string.include?(';')
path_string.split(separator).detect do |dir_path|
oc_path_dir = path_string.split(separator).detect do |dir_path|
File.exist? "#{dir_path}\\oc.exe"
end

oc_path_dir.nil? ? nil : oc_path_dir + '\oc.exe'
Copy link
Contributor

Choose a reason for hiding this comment

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

We let this sneak in for now ;-)

Copy link
Contributor Author

@brgnepal brgnepal Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@hferentschik I had already explained it 😄

So, in windows, devsuite download the oc binary and place it in C:\DevSuite\cdk\bin\oc.exe which is also added in environment variable $PATH.

Previously, I was only checking through the directory paths presents in $PATH and checking the existence of dir_path + '\oc.exe' file. On first detection, I return the dir_path only. However, I need the actual full path not dir_path.
Now, sometime if detect doesn't match anything then it returns nil.

Probably, I should have changed this line to

oc_path_dir + '\oc.exe' unless oc_path_dir.nil?

end
end
end
Expand Down
6 changes: 5 additions & 1 deletion locales/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ en:
A supported service. For example: docker, kubernetes or openshift.

Options:
-h, --help print this help
-h, --help print this help
--cli-version binary version to install
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not providing any short options for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hferentschik , no. Do you think it is mandatory? we have other options like --script-readable etc too which doesn't have short options

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me. Just wanted to make sure we are not missing something.

--path absolute or relative path where to install the binary

Example:
vagrant service-manager install-cli docker
Expand Down Expand Up @@ -199,6 +201,8 @@ en:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli %{service} | tr -d '\r')"
service_not_enabled: |-
'%{service}' service is not enabled.
invalid_binary_path: |-
Directory path %{dir_path} is invalid or doesn't exist.
unsupported_version: |-
Something went wrong to find the correct download link.
We recommend you to use '--cli-version' for your desired client version.
Expand Down
27 changes: 12 additions & 15 deletions test/vagrant-service-manager/installer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,15 @@ module ServiceManager

describe 'Docker' do
before do
@type = :docker
@options = { box_version: 'adb', '--cli-version' => '1.11.0' }
@installer = Installer.new(@type, @machine, @machine.env, @options)
@options = { type: :docker, box_version: 'adb', '--cli-version' => '1.11.0' }
@installer = Installer.new(@machine, @machine.env, @options)
end

it 'should set default values properly' do
@installer.instance_variable_get('@type').must_equal @type
@installer.instance_variable_get('@type').must_equal @options[:type]
@installer.instance_variable_get('@machine').must_equal @machine
@installer.instance_variable_get('@env').must_equal @machine.env
@installer.instance_variable_get('@box_version').must_equal 'adb'
@installer.instance_variable_get('@options').must_equal(@options)
end

it 'should build handler class dynamically' do
Expand All @@ -48,16 +47,15 @@ module ServiceManager

describe 'OpenShift' do
before do
@type = :openshift
@options = { box_version: 'adb', '--cli-version' => '1.2.0' }
@installer = Installer.new(@type, @machine, @machine.env, @options)
@options = { type: :openshift, box_version: 'adb', '--cli-version' => '1.2.0' }
@installer = Installer.new(@machine, @machine.env, @options)
end

it 'should set default values properly' do
@installer.instance_variable_get('@type').must_equal @type
@installer.instance_variable_get('@type').must_equal @options[:type]
@installer.instance_variable_get('@machine').must_equal @machine
@installer.instance_variable_get('@env').must_equal @machine.env
@installer.instance_variable_get('@box_version').must_equal 'adb'
@installer.instance_variable_get('@options').must_equal(@options)
end

it 'should build handler class dynamically' do
Expand All @@ -76,16 +74,15 @@ module ServiceManager

describe 'Kubernetes' do
before do
@type = :kubernetes
@options = { box_version: 'adb' }
@installer = Installer.new(@type, @machine, @machine.env, @options)
@options = { type: :kubernetes, box_version: 'adb' }
@installer = Installer.new(@machine, @machine.env, @options)
end

it 'should set default values properly' do
@installer.instance_variable_get('@type').must_equal @type
@installer.instance_variable_get('@type').must_equal @options[:type]
@installer.instance_variable_get('@machine').must_equal @machine
@installer.instance_variable_get('@env').must_equal @machine.env
@installer.instance_variable_get('@box_version').must_equal 'adb'
@installer.instance_variable_get('@options').must_equal(@options)
end

it 'should build handler class dynamically' do
Expand Down