install-cli with --path give wrong export

[jeffmaury]

OS details:
Win10 Pro

Provider:
VirtualBox

Output of vagrant -v:

Vagrant 1.7.4

Output of vagrant plugin list:

vagrant-registration (1.3.1)
  - Version Constraint: 1.3.1
vagrant-service-manager (1.4.1)
  - Version Constraint: 1.4.1
vagrant-share (1.1.5, system)
vagrant-sshfs (1.2.1.b3eae35)
  - Version Constraint: 1.2.1.b3eae35

Output of vagrant service-manager box version:

Container Development Kit (CDK) 2.2

Steps to reproduce the issue:

1. Go to the CDK vagrant folder
2. mkdir cli
3. vagrant service-manager install-cli openshift --path cli

Describe the results you received:

# Binary now available at /cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose/cli
# run binary as:
# oc.exe <command>
export PATH=/cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose:$PATH

# run following command to configure your shell:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli openshift --path cli | tr -d '\r')"

Describe the results you expected:

# Binary now available at /cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose/cli
# run binary as:
# oc.exe <command>
export PATH=/cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose/cli:$PATH

# run following command to configure your shell:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli openshift --path cli | tr -d '\r')"

[brgnepal]

As per the doc https://github.com/projectatomic/vagrant-service-manager/blob/master/commands.adoc#install-cli

the command that need to run should be

vagrant service-manager install-cli openshift --path cli/oc

@jeffmaury Could you check that and add result?

[jeffmaury]

With --path cli/oc, then the result is:

# Binary now available at /cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose/cli/oc
# run binary as:
# oc.exe <command>
export PATH=/cygdrive/c/Users/JeffMAURY/CDK/2.2/components/rhel/rhel-ose/cli:$PATH

# run following command to configure your shell:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli openshift --path cli\oc | tr -d '\r')"

[mlabuda]

I tried on linux (FC24) to install oc, but neither relative path nor absolute path did work. Tried following 2 commands:
vagrant service-manager install-cli openshift --path=/home/mlabuda/cdk/oc
vagrant service-manager install-cli openshift --path=./oc

I've tried to use it as path to a directory (/home/mlabuda/cdk/oc dirs exists| and also as a path to binary (without existence of oc directory in cdk dir). Client tools were not downloaded to any of those paths/dirs. Both commands gimme following output:

# Binary already available at /home/mlabuda/.vagrant.d/data/service-manager/bin/openshift/1.2.1/oc
# run binary as:
# oc <command>
export PATH=/home/mlabuda/.vagrant.d/data/service-manager/bin/openshift/1.2.1:$PATH

# run following command to configure your shell:
# eval "$(VAGRANT_NO_COLOR=1 vagrant service-manager install-cli openshift  | tr -d '\r')"

[brgnepal]

@mlabadu Actually in your case binary has been already downloaded and hence it is showing as
Binary already available at ...

Could you do rm -rf /home/mlabuda/.vagrant.d/data/service-manager and run the command?

[mlabuda]

I tried again with clearing dirs after each attempts. All 4 scenarios end up same way - it was always put into /home/mlabuda/.vagrant.d/data/service-manager/bin/openshift/1.2.1/oc

Also notice, that downloaded version of oc binary is 1.2.1 even though I use CDK 21-Nov-2016.rc2. There should be binary 1.3.1 instead of 1.2.1.

[brgnepal]

@jeffmaury @mlabuda I forgot to mention that this option --path is not supported for CDK since it is not possible to get the version matched between the oc binary with the OSE server. It was only available for ADB.

Probably adding an information message like "Not supported for CDK" will be good approach here.

Anyway thanks for coming up with this.

[hferentschik]

I forgot to mention that this option --path is not supported for CDK since it is not possible to get the version matched between the oc binary with the OSE server. It was only available for ADB.

@budhrg it's the version which we cannot change on CDK, right? The path option should work on ADB and CDK regardless. IMO this is a bug.

[brgnepal]

@jeffmaury @mlabuda I forgot to mention that this option --path is not supported for CDK

I was wrong with my statement here. --path path does supported for CDK but only caveat is that the oc version will be 1.2.1.

Another thing, I found that relative path doesn't work for --path here like cli/oc which Jeff was using.

I will add the fix for the relative path and things will work then.

[hferentschik]

I was wrong with my statement here. --path path does supported for CDK but only caveat is that the oc version will be 1.2.1.

Right. CDK only offers only a single blessed version of oc. Using the --version option should abort with a warning, right?

Another thing, I found that relative path doesn't work for --path here like cli/oc

So that's a bug then, right? We said it should work with absolute and relative path. At least that's what I remember.

[brgnepal]

So that's a bug then, right? We said it should work with absolute and relative path. At least that's what I remember.

Ya

[hferentschik]

That said, relative path might be tricky at times. I guess with relative the user would mean relative to the directory from which he executes 'vagrant service-manager install-cli'. This is not necessarily the base of file operations. I would think the base is somewhere relative to the directory from which the plugin is executed. I would not be surprised if oc is actually somewhere on the file system, just not were one expects. So we need to also define that "relative" means relative to the directory of the Vagrantfile. So doc and code changes required here.

[brgnepal]

@hferentschik I think relative mostly means from the directly where vagrant install-cli command is fired.

[hferentschik]

I think relative mostly means from the directly where vagrant install-cli command is fired.

that's the expectation, but this is not necessarily the case when using the file api of say Ruby or Java.

[brgnepal]

but this is not necessarily the case when using the file api of say Ruby or Java.

Got it. Like our cucumber feature implementation.

[adietish]

@budhrg why is it that one has to add "oc" to the path? Doesnt seem like the most intuitive to me.

[brgnepal]

@adietish Ya agreed, realized later 😉 It has been tracked here now #446

[hferentschik]

why is it that one has to add "oc" to the path? Doesnt seem like the most intuitive to me.

not 'oc' itself, but the path in which oc is located. This way you can change into another directory, for example your project sources and execute any 'oc' command you like.

[hferentschik]

Got it. Like our cucumber feature implementation.

Well, yes and now. But look at https://github.com/projectatomic/vagrant-service-manager/blob/master/lib/vagrant-service-manager/binary_handlers/binary_handler.rb#L85

This code works just for the case where the path is absolute and the directory structure exists. If there are intermediate directories missing it will fail (no mkdir_p). Whether one should do this intermediate create or not is another question. If not, there should be a clear warning/error about the nature of the problem. When it comes to relative path, there is nothing in here which will assure that it will be relative to the Vagrantfile. My guess is that per default it is relative to the directory from which Ruby gets started. Really one needs to check whether one is dealing with a relative path and if so determine the path to the Vagrantfile and create take it from there. Whether you want to support intermediate directories is again an orthogonal issue, but if not, the user needs to get clear and consistent messages.

[hferentschik]

why is it that one has to add "oc" to the path? Doesnt seem like the most intuitive to me.

ahh, now I get it. You seem to need to specify the actual binary as well. Yes, that's wrong. You should just have to specify the directory/path.

[jeffmaury]

You can bu then the PATH is wrongly set

[brgnepal]

This code works just for the case where the path is absolute and the directory structure exists. If there are intermediate directories missing it will fail (no mkdir_p). Whether one should do this intermediate create or not is another question. If not, there should be a clear warning/error about the nature of the problem.

This is an error saying "Invalid path"

[brgnepal]

ahh, now I get it. You seem to need to specify the actual binary as well. Yes, that's wrong. You should just have to specify the directory/path.

Tracked here now #446

[brgnepal]

Closing it as it has been address by #446