Skip to content
Snippets Groups Projects

Add docker support for terminal

Merged Steve Xuereb requested to merge 3-proxy-docker-connection into master
All threads resolved!

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?

gitlab-runner#3467 (closed)

Closes #3 (closed)

Edited by Steve Xuereb

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kamil Trzciński assigned to @SteveAzz

    assigned to @SteveAzz

  • Kamil Trzciński
  • Steve Xuereb added 1 commit

    added 1 commit

    • 7f1ec686 - Add docker support for terminal

    Compare with previous version

  • Author Maintainer

    @ayufan mind if you take another look at this MR I've addressed all the discussion points

  • assigned to @ayufan

  • Steve Xuereb marked the checklist item Added for this feature/bug as completed

    marked the checklist item Added for this feature/bug as completed

  • Steve Xuereb marked the checklist item All builds are passing as completed

    marked the checklist item All builds are passing as completed

  • Steve Xuereb marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

    marked the checklist item Branch has no merge conflicts with master (if you do - rebase it please) as completed

  • @SteveAzz It seems OK. I simply just don't know:

    1. How we gonna inject our messages into streams, as upgraders are implemented by this code,
    2. 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
  • Kamil Trzciński resolved all discussions

    resolved all discussions

  • Kamil Trzciński resolved all discussions

    resolved all discussions

  • Kamil Trzciński
  • Kamil Trzciński assigned to @SteveAzz

    assigned to @SteveAzz

  • Steve Xuereb mentioned in issue #4

    mentioned in issue #4

  • Author Maintainer

    @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 appended 0/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 append 0/1/.. to the stream.

  • assigned to @ayufan

  • Kamil Trzciński assigned to @SteveAzz

    assigned to @SteveAzz

  • @SteveAzz Fair. I asked for one extra change to make this code easier.

  • Kamil Trzciński
  • Steve Xuereb added 1 commit

    added 1 commit

    • d94b10c2 - Add docker support for terminal

    Compare with previous version

  • assigned to @ayufan

  • Kamil Trzciński assigned to @SteveAzz

    assigned to @SteveAzz

  • Steve Xuereb added 1 commit

    added 1 commit

    • 5af59b87 - Add docker support for terminal

    Compare with previous version

  • Author Maintainer

    Sorry for the ping/pong :see_no_evil:

  • assigned to @ayufan

  • Kamil Trzciński resolved all discussions

    resolved all discussions

  • Kamil Trzciński approved this merge request

    approved this merge request

  • mentioned in commit 087cdeae

  • Thanks :tada:!

  • Please register or sign in to reply
    Loading