-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: Implementing resizable/error exec stream #900
Feat: Implementing resizable/error exec stream #900
Conversation
7ded530
to
948e572
Compare
@@ -349,7 +349,16 @@ static int parseExitCode(ApiClient client, InputStream inputStream) { | |||
return -1; | |||
} | |||
|
|||
protected static class ExecProcess extends Process { | |||
public interface ResizableStream { |
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.
What is the purpose of adding these interfaces? Why not just add the methods to ExecProcess
?
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.
What is the purpose of adding these interfaces?
the standard interface java.lang.Process
only exposes access to three streams: stdin/stdout/stderr. while for a complete implementation of "web-terminal", we will have to leave the developers public access to another stream named "resize" which tells the kubernetes exec stream to probably adjust the terminals width&height..
Why not just add the methods to ExecProcess?
it is protected. otherwise how do you think about promote it to a public inner class?
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 guess my feeling is you're going to have to cast it one way or the other:
Process p = Exec.exec(...);
ResizableExecStream res = (ResizableExecStream)p;
res.getResizeStream();
...
vs
ExecProcess p = (ExecProcess) Exec.exec(...);
p.getResizeStream();
I don't see the win from the interfaces, but I could be wrong.
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 reverted the interfaces and made the class ExecProcess
public w/ the new getters, wdyt?
35ee3d5
to
0ebf8d5
Compare
0ebf8d5
to
cdfedcf
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, yue9944882 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@yue9944882 hello, how to usage this ? resize web terminal ExecProcess p = (ExecProcess) Exec.exec(...);
p.getResizeStream(); |
Kubelet is actually exposing a set of channels in the exec webconnection:
this pull adds access for the last two channel
/cc @brendandburns