Skip to content

jbide-23862: fix for spaces in path to oc binary #272

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 22, 2017
Merged

jbide-23862: fix for spaces in path to oc binary #272

merged 3 commits into from
May 22, 2017

Conversation

bdshadow
Copy link

@adietish @jeffmaury @robstryker, i've checked it on Fedora 25 and Windows 10.
Please, check it on MacOS and once again on Linux and Windows.

@adietish
Copy link
Member

[test]

@adietish
Copy link
Member

@bdshadow dare to add unit-tests please?
I'm testing it manually...

adietish
adietish reviewed May 16, 2017
View reviewed changes
...om/openshift/internal/restclient/capability/resources/AbstractOpenShiftBinaryCapability.java Outdated
String cmdLine = new StringBuilder(location).append(' ').append(buildArgs(Arrays.asList(options))).toString();
String[] args = StringUtils.split(cmdLine, " ");
List<String> args = new ArrayList<String>();
args.add("oc");
Copy link
Member

@adietish adietish May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

dont think that we should hard code the binary name. Can't we just take the last segment of the cmd-line?
new File(location).getName() would have the same effect in a generic way

Copy link
Author

Choose a reason for hiding this comment

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

I think you mean location, not cmdLine. Yes, i think it's a good proposal. Will change it. Though anyway we hard code such things, for example:

openshift-restclient-java/src/main/java/com/openshift/internal/restclient/capability/resources/OpenShiftBinaryRSync.java

Line 144 in 83f04c2

final StringBuilder argsBuilder = new StringBuilder("rsync ");

@adietish
Copy link
Member

quite incredible, but even with the fix, which looks fine, things wont work on MacOS:

com.openshift.restclient.OpenShiftException: Does your OpenShift binary location exist? Error starting process: Cannot run program "oc" (in directory "/Users/adietish/Downloads/oc binaries/ocp-3.4.1.1.2"): error=2, No such file or directory
	at com.openshift.internal.restclient.capability.resources.AbstractOpenShiftBinaryCapability.startProcess(AbstractOpenShiftBinaryCapability.java:169)
	at com.openshift.internal.restclient.capability.resources.AbstractOpenShiftBinaryCapability.start(AbstractOpenShiftBinaryCapability.java:153)
	at com.openshift.internal.restclient.capability.resources.OpenShiftBinaryPodLogRetrieval$PodLogs.getLogs(OpenShiftBinaryPodLogRetrieval.java:111)
	at com.openshift.internal.restclient.capability.resources.OpenShiftBinaryPodLogRetrieval.getLogs(OpenShiftBinaryPodLogRetrieval.java:67)
	at org.jboss.tools.openshift.internal.ui.job.PodLogsJob$ConsoleStreamPipe.runOCBinary(PodLogsJob.java:162)
	at org.jboss.tools.openshift.internal.core.OCBinaryOperation.run(OCBinaryOperation.java:34)
	at org.jboss.tools.openshift.internal.ui.job.PodLogsJob$ConsoleStreamPipe.run(PodLogsJob.java:152)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: Cannot run program "oc" (in directory "/Users/adietish/Downloads/oc binaries/ocp-3.4.1.1.2"): error=2, No such file or directory
	at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
	at com.openshift.internal.restclient.capability.resources.AbstractOpenShiftBinaryCapability.startProcess(AbstractOpenShiftBinaryCapability.java:165)
	... 7 more
Caused by: java.io.IOException: error=2, No such file or directory
	at java.lang.UNIXProcess.forkAndExec(Native Method)
	at java.lang.UNIXProcess.<init>(UNIXProcess.java:247)
	at java.lang.ProcessImpl.start(ProcessImpl.java:134)
	at java.lang.ProcessBuilder.start(ProcessBuilder.java:1029)
	... 8 more

@bdshadow
Copy link
Author

@adietish yes, it's really strange. So now it works everywhere except MacOs

@adietish
Copy link
Member

adietish commented May 22, 2017 •
edited
Loading

@bdshadow great, invoking now works for port forwarding and rsync when launched from JBT.
I have now troubles invoking pod logs which outputs the following:

OpenShiftBinaryCapability process exited: error: expected 'logs POD_NAME [CONTAINER_NAME]'.
POD_NAME is a required argument for the logs command
See 'oc logs -h' for help and examples.

The command that's built - and works fine on the cmd-line - is the following:

/Users/adietish/.minishift/cache/oc/v3.5.5.8/oc logs --insecure-skip-tls-verify=true --server=https://192.168.64.2:8443  hola-1-1pscn -n hola --token=oFhZZrGozk3KyzZ23-Js0LLMyBtAwHPUSbzcBmfABlU  -f  -c hola

the arguments that are built are the following:

[/Users/adietish/.minishift/cache/oc/v3.5.5.8/oc, logs, --insecure-skip-tls-verify=true, --server=https://192.168.64.2:8443, , hola-1-1pscn, -n, hola, --token=oFhZZrGozk3KyzZ23-Js0LLMyBtAwHPUSbzcBmfABlU, , -f, , -c, hola]

adietish
adietish requested changes May 22, 2017
View reviewed changes
...om/openshift/internal/restclient/capability/resources/AbstractOpenShiftBinaryCapability.java
String[] args = StringUtils.split(cmdLine, " ");
ProcessBuilder builder = new ProcessBuilder(args);
List<String> args = new ArrayList<String>();
ProcessBuilder builder = null;
Copy link
Member

Choose a reason for hiding this comment

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

i believe, that for improved readability, we could have l161-l177 extracted to their own method

ProcessBuilder createProcessBuilder(String location, OpenShiftBinaryOption... options)

@bdshadow
Copy link
Author

@adietish a strange comma is here in the middle:

--server=https://192.168.64.2:8443, , hola-1-1pscn,

@bdshadow
Copy link
Author

@adietish fixed, check it out, please

@openshift-bot
Copy link
Contributor

Evaluated for javaclient test up to dcb2df2

@openshift-bot
Copy link
Contributor

Java Client Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test-openshift-restclient-java/325/) (Base Commit: 645fbbf)

@adietish
Copy link
Member

adietish commented May 22, 2017 •
edited
Loading

great!
works for me now (on Mac) when I have the oc binary on a path with spaces. Being tenacious payed out and you & we won :)
image

@adietish
Copy link
Member

[merge]

@adietish
Copy link
Member

[merge]

@adietish adietish merged commit 19bd9fd into openshift:master May 22, 2017
@adietish
Copy link
Member

deployed openshift-restclient-java 5.7.0-SNAPSHOT to repository.jboss.org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adietish adietish adietish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bdshadow @adietish @openshift-bot