-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: first draft of copy dsl for pods. #1444
Conversation
Can one of the admins verify this patch? |
@rohanKanojia: I managed to solve the issue that we were having. It seems that the root cause was that the exec command was failing silently and thus nothing was written in the streams. So, I removed the commit, with the experiments on the ExecWebSocketListener, timeouts etc, since it was not required for this to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with some minor comments, Thank you.
@@ -15,6 +15,9 @@ | |||
*/ | |||
package io.fabric8.kubernetes.client.dsl.internal; | |||
|
|||
import java.io.File; | |||
import java.io.FileNotFoundException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import ;-)
|
||
private InputStream readFile(String source) throws Exception { | ||
//Let's wrap the code to a callable inner class to avoid NoClassDef when loading this class. | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use lambdas :-) here? Since we're already on Java 8...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not!
return new Callable<InputStream>() { | ||
@Override | ||
public InputStream call() throws Exception { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception is never thrown in this block
|
||
// | ||
// | ||
// The copy and read utilities below have been inspired by Brendan Burns copy utilities on the offical kubernetes-client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo s/offical/official
Did you mean this PR: kubernetes-client/java#375 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
||
import static junit.framework.TestCase.assertNotNull; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.awaitility.Awaitility.await; | ||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import :-)
[merge] |
No description provided.