-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 improvements #13714
Dev container improvements #13714
Conversation
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.
Thank you very much for this great evolution of the devcontainer support!
I tested this change with the provided Dockerfile
and devcontainer.json
and observed two issues:
- The environment in the container wasn't set correctly; that is the line
ENV PATH="/go/bin:${PATH}"
didn't seem to have an impact so thatgo
wasn't available on the path. - The port forwarding didn't seem to work. Even though the
devcontainer.json
specifies aforwardPorts
, this port didn't show up in the Ports view. If I try to manually add it, I get anEADDRINUSE
error in the console. Before and after I tried adding it to the Port view, the port wasn't reachable from the host.
Maybe this is also related to something local for me, but I'm not sure. Do you have an idea?
Thanks again!
@injectable() | ||
export class PreferenceFrontendContribution implements FrontendApplicationContribution { | ||
@inject(CliPreferences) | ||
protected readonly CliPreferences: CliPreferences; |
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.
protected readonly CliPreferences: CliPreferences; | |
protected readonly cliPreferences: CliPreferences; |
protected readonly preferenceService: PreferenceService; | ||
|
||
onStart(): void { | ||
this.CliPreferences.getPreferences().then(async preferences => { |
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.
this.CliPreferences.getPreferences().then(async preferences => { | |
this.cliPreferences.getPreferences().then(async preferences => { |
Thats weird both go and the forwarded port worked fine for me locally. Did you by any chance have a service allready running on 1313 thats the only thing i could explain this with. |
Thanks for the fast response! I'll double-check at latest tomorrow and report back. I think I made sure to kill and prune all containers in order to test the setup. But I'll check. |
I can confirm that Unfortunately, however, I don't really have better news after retrying from scratch regarding the
When I close Theia, 1313 remains blocked and Comparing to VS Code, I see that I didn't debug into all of that in more detail, but in summary I have the following observations:
I hope this helps! If it doesn't, please let me know. I'm happy to try provide more info or if you like even jump on a short screenshare. Thank you for all your work on this! |
thanks for this in depth analysis. I'll take a look today |
@planger Small update: I am able to reproduce the problem with the PATH. It seems it works fine when using a docker exec instance (for example when using the docker desktop exec tab) but not when opening a terminal through theia. Still no idea why though yet. But seems it has something to do with the temrinal library theia is using. Launching an application from theia also has the correct path Regarding the port forwarding it works fine for me. But by default there is no service running in my container listening on port 1313 so i had to start one. Also from the Dockerfile it seems there is no service started |
@jonah-iden Thanks for the update and great you have a lead for the path issue! Regarding the port: Yeah the Dockerfile / devcontainer doesn't start a process on the port automatically, but only if you run |
I Found the issue with the PATH environement. It seems the /etc/profile file was overwriting the PATH and thus removing the docker changes. VSCode seems to be modifiying the profile file so that the path is not overwritten. |
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.
Thank you very much @jonah-iden for the great work! I can confirm that the latest changes fix the following issues
go
is now correctly on thePATH
and the build task works perfectly fine! 💯- the container is correctly stopped when I close Theia 🥳
- the ports now show up in the ports view 👁️
I still have the issue that the port is not available on the host. As you say this is working for you, I think it is worth pulling in another test to ensure it is not something specific to me.
@JonasHelming Can you please test if it works for you to open a container based on the test devcontainer file and run hugo server
(with a hugo project in the workspace) and see if you can access port 1313
from your host's browser?
Thank you!
the problem may be, that currently the forwarded ports are forwarded via the docker container. VSCode is using their own dynamic port forwarding mechanism for this. Could it be the hugo server is running on localhost and not 0.0.0.0. Maybe that could explain it? |
Excellent hint! That is indeed the reason. By default Do you think we should also look into replicating this behavior of VS Code or is it too much effort? |
The dynamic forwarding feature allready exists for theia, so it shouldn't be to difficult to change this behaviour. |
Right, I agree. For compatibility reasons, I think it makes sense to also support that in Theia. Thank you very much! |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
PostCreateCommand, RemoteUser, sttings and extensions Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
…n launching a terminal 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>
78fe433
to
738d3c0
Compare
|
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.
Thank you very much! The dynamic port forwarding works like a charm. This PR is an excellent addition to the dev-container support. Thank you!
What it does
This adds 4 new supported properties of the devcontainer JSON.
remoteUser
which sets the user theia is started with in the containerpostCreateCommand
a command executed after creating and initally starting the containerextensions
to set extensions installed when creating the containersettings
preferences to set when creating the containerAlso adds a new command line option to theia which sets preferences on startup.
How to test
Here is a devcontainer.json and a Dockerfile for testing.
devcontainer.json
Dockerfile
Put both of these in a
.devconainer
folder and runDev Container: Reopen in Container
When creating the container in the output tab you should see the output of the postCreateCommand at the end
In the opened Container the following properties should be set:
bin/bash
) should benode
html.format.templating
property should be set to trueFollow-
Review checklist
Reminder for reviewers