Add docker support for terminal
What does this MR do?
Add support of interactive web terminal for the docker
executor.
Are there points in the code the reviewer needs to double check?
The following MR takes the simplistic approach on how to proxy the docker and the WebSocket. There is a more detailed approach at this branch which I am not sure if it is better or not.
If we go with this approach we can refactor the FileDescriptorProxy
and DockerProxy
to be a new ReadWriterProxy
but this will have backward incompatibility consequences, which might not be to much of an issue since we only have 1 consumer of this package thus far.
Does this MR meet the acceptance criteria?
-
Documentation created/updated - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Branch has no merge conflicts with master
(if you do - rebase it please)
What are the relevant issue numbers?
Closes #3 (closed)
Merge request reports
Activity
mentioned in merge request gitlab-runner!1008 (merged)
- Resolved by Kamil Trzciński
- Resolved by Kamil Trzciński
- Resolved by Kamil Trzciński
- Resolved by Kamil Trzciński
@ayufan mind if you take another look at this MR I've addressed all the discussion points
assigned to @ayufan
@SteveAzz It seems OK. I simply just don't know:
- How we gonna inject our messages into streams, as upgraders are implemented by this code,
- I just wonder if the better interface would not be that
gitlab-terminal
provides a decoded stream with clear messages that we can forward to where we need, so this also allows us to send messages.
It is OK, minimal, I simply don't think that this is sufficient change for us to solve the above goals: have an ability to use our own streams to bi-directional communication with Frontend :(
Anyway, I'm fine on iterating. Lets see how it evolves.
Edited by Kamil Trzciński- Resolved by Kamil Trzciński
- Resolved by Kamil Trzciński
mentioned in issue #4
@ayufan very good points for !3 (comment 103406023) and !3 (comment 103404566) ! I have created a separate issue #4 to tackle these, as they seem like a v2 of our protocol, since right now our proctol seems to be purly for bi-directional text forwarder for the terminal, even for the k8s stream we strip down the prefix as well if we look at this block if we look at gitlab-org/gitlab-ce#50384 we discussed to introduce
v2.terminal.gitlab.com
for the notifications which I think here we should introduct the appended0/1
to differentiate between the stream and notifications.I would say for the notifications we would introduce a new exposed struct that holds the stream of data and a channel where we are able to send notifications to so like think i tink it will be a bit simpler and then it is up to the
gitlab-terminal
packag to append0/1/..
to the stream.assigned to @ayufan
- Resolved by Kamil Trzciński
assigned to @ayufan
- Resolved by Kamil Trzciński
- Resolved by Kamil Trzciński
assigned to @ayufan
mentioned in commit 087cdeae