Skip to content
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

Dev-Container support #13372

Merged
merged 17 commits into from
Mar 15, 2024
Merged

Dev-Container support #13372

merged 17 commits into from
Mar 15, 2024

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Feb 9, 2024

What it does

This implements a first version of the Dev-Container Support vscode offers for Theia. This can later also be used to forward arbritrary ports at container runtime

The devcontainer.json format offers alot of configuration option of which currently only a certain few are implemented.

Implemented properties are:

  • image
  • dockerfile/build.dockerfile (dockerfile support still needs some more testing and work regarding its options)
    • including build.args which are just passed to docker as is
  • forwardPorts
  • mounts

The communication with the theia backend inside the container is done through a docker exec instance to the ´dev-conatiner-server.ts´ which forwads all communcation to stdio.

Support for having multiple devcontainer files is also added

I would love some feedback on this, especially what missing features would be most important to make it most useful.

How to test

You need docker installed and started on your machine

  1. open a folder in theia
  2. create a new .devcontainer folder at the root
  3. add a devcontainer.json and neccessary resources for it
    here is an example file using a image wich is automaticly pulled by docker
{
    "name": "test_container",
    "image": "nikolaik/python-nodejs:python3.11-nodejs20-slim",
    "forwardPorts": [4000, "3000:3003"],
    "mounts": [{"source": "some/path", "target": "/workspaces", "type": "bind" }]

}

or here is a dockerfile with its respective devcontainer.json file for testing dockerfile support:
devcontainer.json
Dockerfile.txt

  1. execute Dev Containers: Reopen in Container
  2. your workspace should then open inside the container

Follow-ups

Things not implemented yet which vscodes supports:

  • Port forwarding at runtime (this feature is in general still missing in the remote feature) (its mostly done though, there will be a PR as soon as this one is merged)
  • attaching to running containers
  • Docker Compose support
  • variables inside dockerconfig.json
  • Showing the docker ouput, especially for when images are build through dockerfiles or pulled
  • lots of smaller configuration options

Review checklist

Reminder for reviewers

@jonah-iden jonah-iden force-pushed the jide/dev-container-support branch from 7e8921d to eb8312f Compare February 9, 2024 14:22
@jonah-iden jonah-iden marked this pull request as ready for review February 21, 2024 09:30
@jonah-iden jonah-iden force-pushed the jide/dev-container-support branch from 54c5a43 to 3f4f058 Compare February 21, 2024 10:56
@JonasHelming
Copy link
Contributor

@msujew : Once you give your approval, we will do another test, does this work for you?

@jonah-iden jonah-iden mentioned this pull request Mar 1, 2024
1 task
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
…on files

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden jonah-iden force-pushed the jide/dev-container-support branch from 56ceb78 to 4a18a6f Compare March 1, 2024 09:12
@jonah-iden
Copy link
Contributor Author

@JonasHelming Since Mark is on vacation for the rest of the Month, feel free to test it and give feedback if you find the time

@JonasHelming JonasHelming requested a review from planger March 6, 2024 20:28
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you very much @jonah-iden! This is great work and already works pretty nicely.

My biggest wish would be to show the output of the docker container, to help diagnosing issues. Currently it is very hard to debug situations where the container doesn't start for some reasons, which may happen quite easily since we don't support the full devcontainer format yet.

Aside from that I have a security related question: Afais we don't protect the exposed port of the Theia backend running in the container from the outside world, or did I miss that?
If we don't, I suggest to bind at least the Theia backend port only to the local IP address by default to avoid having it exposed to the entire network.

Other than that I only have a few minor nitpicks inline.

Thanks again for your great work on this feature! This is truly a game-changer, and I'm eager to see it evolved incrementally to support more and more properties of the devcontainer format.


The `@theia/dev-container` extension provides functionality to create, start and connect to development containers similiar to the
[vscode Dev Containers extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode-remote.remote-containers).

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to document which properties of the devcontainer format are supported at the moment or link to an issue that lists the unsupported ones.

@jonah-iden
Copy link
Contributor Author

Thanks for the review! I'll try to add showing the output today

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden
Copy link
Contributor Author

@planger added a for now very basic output display. There is some messages not looking super nice but that can be improved later on

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@JonasHelming
Copy link
Contributor

@jonah-iden When this is ready for re-reviewe, please just trigger Philips review again

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden jonah-iden requested a review from planger March 15, 2024 11:54
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you for the additions @jonah-iden! This looks great and I think it should be good to go as a first version of this excellent feature.

@jonah-iden jonah-iden merged commit 401d4b9 into master Mar 15, 2024
14 checks passed
@github-actions github-actions bot added this to the 1.48.0 milestone Mar 15, 2024
@msujew msujew added the remote issues related to the remote functionality label Apr 30, 2024
@msujew msujew deleted the jide/dev-container-support branch April 30, 2024 13:02
@hsnoil
Copy link

hsnoil commented May 29, 2024

@jonah-iden for this patch, are ENV variables like ${localWorkspaceFolder} and ${localEnv:HOME} suppose to work for mounting? Because it keeps saying add absolute path

https://code.visualstudio.com/remote/advancedcontainers/add-local-file-mount

@jonah-iden
Copy link
Contributor Author

jonah-iden commented May 29, 2024

@hsnoil no sorry, variables in the devcontainer.json are not yet implemented for dev-containers.

I took a small look at it. Seems like implementing this would require a refactor or similar of the VaraibleResolverService so that it would be somehow available in the backend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
remote issues related to the remote functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants