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

brgnepal · 2016-10-17T07:05:25Z

Updated help for install-cli
Also added checks to validate path given with --path option
Added documentation for --cli-version and --path options

brgnepal · 2016-10-17T07:05:58Z

@hferentschik Lets get rid of this PR 😄

hferentschik · 2016-10-17T08:15:47Z

features/install-cli.feature

    Examples:
      | box   | provider   | ip          |
      | adb   | virtualbox | 10.10.10.42 |
      | adb   | libvirt    | 10.10.10.42 |
+      | cdk   | virtualbox | 10.10.10.42 |


Didn't we say we keep the CDK tests in cdk-openshift? I am trying to remember what the plan was?

hferentschik · 2016-10-17T08:17:19Z

features/install-cli.feature

+    And the binary "docker" should be installed with version "1.12.1"
+
+    # '../vagrant.d' maps to ENV['VAGRANT_HOME']
+    When I run `bundle exec vagrant service-manager install-cli docker --path ../vagrant.d/docker`


I am still skeptical about ../vagrant.d/docker. Have you tried "#{ENV['VAGRANT_HOME']/docker}?

@hferentschik , it will not work as #{ENV['VAGRANT_HOME']/docker} is not know through command line. Even when passing $VAGRANT_HOME/docker, ruby tried to create $VAGRANT_HOME directory inside aruba directory.

hferentschik · 2016-10-17T08:19:28Z

commands.adoc

@@ -236,11 +244,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/docker


What's about /home/johndoe/bin/docker as an example dir. No one puts a binary directly into the home directory, right?

@hferentschik Ok agreed

hferentschik · 2016-10-17T08:20:36Z

.rubocop_todo.yml

@@ -55,3 +55,12 @@ Style/Documentation:
 Style/FileName:
  Exclude:
    - 'lib/vagrant-service-manager.rb'
+


I think there should be no more Rubocop changes required in this pull request, right? Also this seems to create a merge conflict when applying to master.

@hferentschik Yes

hferentschik · 2016-10-17T08:21:25Z

locales/en.yml

@@ -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


we are not providing any short options for this?

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

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

hferentschik · 2016-10-17T08:27:50Z

.rubocop_todo.yml

+     - 'test/vagrant-service-manager/binary_handlers/*.rb'
+
+Lint/Eval:
+  Enabled: false


I guess this might be needed, right?

Yes @hferentschik

brgnepal · 2016-10-17T09:00:21Z

@hferentschik All tests passed 😄

hferentschik

Looking good. I'd suggest to make one more change to the way the features are written, but otherwise ok now.

hferentschik · 2016-10-17T10:54:19Z

features/adb-openshift.feature

@@ -57,6 +57,12 @@ Feature: Command output from various OpenShift related commands in ADB
    Then the exit status should be 0
    And the binary "oc" should be installed

+    # '../vagrant.d' maps to ENV['VAGRANT_HOME']
+    When I run `bundle exec vagrant service-manager install-cli openshift --cli-version 1.3.0 --path ../vagrant.d/oc`


Here is what I came up with. The issue is really that Aruba does not execute the command in backticks directly and hence interpolation will take place. What's about:

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"

To make this work we need a new step defintion:

# 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

Note, I also changed the way ' And the binary should be installed in path' is implemented:

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

With this in place one can consistently use '#{ENV['VAGRANT_HOME']}' (or any other variable really). I think this is a "cleaner" solution. Everything is explicit. WDYT?

@hferentschik This works great 👍 💯

I didn't know that 😄 Thanks

hferentschik · 2016-10-17T10:54:34Z

commands.adoc

-$ 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:


hferentschik · 2016-10-17T10:55:12Z

locales/en.yml

@@ -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


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

hferentschik · 2016-10-17T12:41:16Z

features/cdk-openshift.feature

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


hmm, is this right? Should it not be "#{ENV['VAGRANT_HOME']}/oc"?

I would expect this to fail, tbh

hferentschik · 2016-10-17T14:30:49Z

commands.adoc

@@ -223,10 +223,18 @@ 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] [arg]` +


Do you really need '[arg]' here? The value for an option is really encapsulated in [options] itself, right? I guess one can do something like '--cli-version=' to indicate that the option takes a value.

@hferentschik --cli-version= will create more confusion IMO as user will then assume they need to enter option value as --cli-version=1.3.0 which VSM cannot parse ATM.

Or you think the options description should include the value as:

The following options can be used:

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

That latter works as well for me. ' [options] [arg]' seems wrong. OR what do you think?

As it stands it is wrong '[options] [arg]'. It implies that options and args are disconnected

@hferentschik updated the PR with the changes.

hferentschik · 2016-10-17T15:00:54Z

features/support/env.rb

+end
+
+Then(/^the binary "([^"]*)" should be installed with version "([^"]*)"$/) do |binary, version|
+  binary_path = "#{ENV['VAGRANT_HOME']}/data/service-manager/bin/#{version}/#{binary}"


I was wrong, we cannot build the path directly. This test fails now, there is one more level after 'bin' which depicts the service. We either would need to pass the service as well to build the full path or we indeed have to do some path matching. Maybe it is more explicit to so something like with something like:
"Then(/^the binary "([^"])" of service "([^"])" should be installed with version "([^"]*)"$/)"? I just want to make sure we don't get any false positives by using a recursive scan approach.

brgnepal · 2016-10-17T16:17:30Z

@hferentschik Pushed the new changes which include missing service name in step definition alongwith tests fix

brgnepal · 2016-10-17T17:25:33Z

@hferentschik CI seems to be passing now 👍

hferentschik · 2016-10-17T21:02:25Z

features/cdk-openshift.feature

@@ -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"


👍 I think this is better

Fix #326 and #327 * Updated help for install-cli * Also added checks to validate path given with --path option * Added documentation for --cli-version and --path options

brgnepal · 2016-10-18T08:00:18Z

@hferentschik Anything remaining in this PR now? I think it is ready to merge now.

hferentschik · 2016-10-18T08:47:01Z

commands.adoc

@@ -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]` +


hferentschik · 2016-10-18T08:48:57Z

features/adb-openshift.feature

@@ -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"


👍

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

hferentschik · 2016-10-18T08:50:01Z

features/adb-openshift.feature

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


👍 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.

hferentschik · 2016-10-18T08:51:07Z

features/install-cli.feature

@@ -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"


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

hferentschik · 2016-10-18T08:51:35Z

lib/vagrant-service-manager/plugin_util.rb

          File.exist? "#{dir_path}\\oc.exe"
        end
+
+        oc_path_dir.nil? ? nil : oc_path_dir + '\oc.exe'


We let this sneak in for now ;-)

@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?

hferentschik · 2016-10-18T10:03:48Z

Yeah :-)

hferentschik suggested changes Oct 17, 2016

View reviewed changes

hferentschik reviewed Oct 17, 2016

View reviewed changes

brgnepal changed the title ~~Fix #326: Tests and docs for --cli-version and --path options~~ Fix #327 and #326 Tests and docs for --cli-version and --path options Oct 17, 2016

hferentschik suggested changes Oct 17, 2016

View reviewed changes

hferentschik reviewed Oct 17, 2016

View reviewed changes

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

7a3f4b1

Fix #326 and #327 * Updated help for install-cli * Also added checks to validate path given with --path option * Added documentation for --cli-version and --path options

hferentschik approved these changes Oct 18, 2016

View reviewed changes

hferentschik merged commit 7a3f4b1 into projectatomic:master Oct 18, 2016

This was referenced Oct 18, 2016

Add tests for install-cli for CDK #327

Closed

Implement the --cli-version and --path options for install-cli #326

Closed

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

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

Conversation

brgnepal commented Oct 17, 2016 • edited Loading

brgnepal commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal commented Oct 17, 2016

hferentschik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal commented Oct 17, 2016

brgnepal commented Oct 17, 2016

Choose a reason for hiding this comment

brgnepal commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brgnepal Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

hferentschik commented Oct 18, 2016

brgnepal commented Oct 17, 2016 •

edited

Loading

brgnepal Oct 17, 2016 •

edited

Loading

brgnepal Oct 17, 2016 •

edited

Loading

brgnepal Oct 17, 2016 •

edited

Loading

brgnepal Oct 18, 2016 •

edited

Loading