-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support remote port forwarding #13439
Conversation
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>
3c2f777
to
deeca28
Compare
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>
deeca28
to
c98a4e2
Compare
Ensure that the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item. Fixes #13076 Contributed on behalf of STMicroelectronics
…is (#13380) By default, when running Theia in Electron, all endpoints are protected by the ElectronTokenValidator. This patch allows accessing the '/metrics' endpoint without a token, which enables us to collect metrics for performance analysis. For this, ElectronTokenValidator is extended to allow access to the metrics endpoint. All other endpoints are still protected. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
* fixed renameing of open notebooks Signed-off-by: Jonah Iden <jonah.iden@typefox.io> * fixed moving of notebook editors to other areas Signed-off-by: Jonah Iden <jonah.iden@typefox.io> --------- Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Since a recent enhancement/refactoring of @theia/playwright, to permit using it in Theia Electron applications, the way to load an application has changed. This commit is an attempt to update the examples that are part of the documentation. I validated the changes in the "theia-playwright-template" repository, and so I have adapted the sample code to that repo's linting rules (using single quotes instead of double). It's possible that other things have changed, that I have not yet encountered, but this should be a good step forward, at least for those just getting started integrating playwright to test their Theia-based app. Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
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>
c98a4e2
to
1ab4a36
Compare
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@JonasHelming @planger here is the dev-container follow up PR for the remote port forwarding at runtime. Should work in general for the remote feature. So both fort SSH and Dev-Containers. |
Cool, thanks! I'm happy to take a look at it next week. |
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 for adding this great new feature to the remote container support! Overall it looks and works very well.
I'd like to mention a few observations below and in the code inline.
The padding of the message "No forwarded ports..." in the widget seems to be missing. E.g. in the Problem view, when no problems are found, this message has a padding of 13px, whereas in the Ports view, this message has no padding at all.
There seems to be a minor focus issue in the Port view:
root WARN Widget was activated, but did not accept focus after 2000ms: port-forwarding-widget
When specifying localhost:<port>
in the port column, it correctly binds only to localhost
, thus it is not reachable with the external IP address. If specified without e.g. localhost:
, it is reachable from the outside. This is great! 👍
However in the UI, this difference doesn't show. Afaik, it also doesn't show in VS Code, but imho it should (e.g. to avoid wondering why you can't access the port when it is bound to a specific IP address). This isn't the most important thing though and certainly shouldn't block merging this. Just thought it might be easy to add in some way.
packages/remote/src/electron-browser/port-forwarding/port-forwarding-widget.css
Outdated
Show resolved
Hide resolved
packages/remote/src/electron-browser/port-forwarding/port-forwarding-widget.tsx
Outdated
Show resolved
Hide resolved
packages/remote/src/electron-browser/port-forwarding/port-forwarding-widget.tsx
Outdated
Show resolved
Hide resolved
packages/dev-container/src/electron-node/docker-container-creation-service.ts
Outdated
Show resolved
Hide resolved
packages/dev-container/src/electron-node/docker-container-creation-service.ts
Outdated
Show resolved
Hide resolved
packages/dev-container/src/electron-node/docker-container-creation-service.ts
Outdated
Show resolved
Hide resolved
packages/remote/src/electron-browser/port-forwarding/port-forwarding-widget.tsx
Outdated
Show resolved
Hide resolved
packages/remote/src/electron-node/remote-port-forwarding-provider.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden Is this ready for a re-review? |
@JonasHelming im so sorry. Yes it is. I totally forgot to request the rereview |
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.
Thanks @jonah-iden for the improvements!
These two observations of my previous review, however, can still be observed and should hopefully be easy to address, before we can merge this change:
The padding of the message "No forwarded ports..." in the widget seems to be missing. E.g. in the Problem view, when no problems are found, this message has a padding of 13px, whereas in the Ports view, this message has no padding at all.
There seems to be a minor focus issue in the Port view:
root WARN Widget was activated, but did not accept focus after 2000ms: port-> forwarding-widget
My earlier comment of indicating that a port is bound to an IP address can be postponed, I guess, as it is also not shown in VS Code.
ups sorry. I guess i must have overlooked those comments. I'll take another look later today |
Great, thank you very much @jonah-iden! |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@planger i changed the requested stuff. Also when not giving an address like localhost, it should now show as 0.0.0.0 to indicate its reachable from the outside. Is that ok like that? |
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.
Excellent, thank you very much @jonah-iden! To me writing 0.0.0.0 if the IP address isn't bound makes a lot of sense. Padding and focus is now fixed as well. Thank you!
What it does
Builds on top #13372. So that PR should probably be merged first
This adds the ability to the remote feature to forward ports to the remote system. Works for both ssh and dev-container connections.
Adds a new Port Widget to manage forwarded Ports.
How to test
Follow-ups
Review checklist
Reminder for reviewers