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

Propagate new Trace Server url upon change #158

Merged

Conversation

hoangphamEclipse
Copy link
Contributor

@hoangphamEclipse hoangphamEclipse commented Jul 4, 2023

This commit allow the VSCode extension to listen to the onDidChangeConfiguration event provided by VSCode and propagate the new URL to components. The new URL is sent as a message to the front end, then the TraceServerUrlProvider will run handlers that was registered to the TSPClientProvider.

Part of #10.

@hoangphamEclipse hoangphamEclipse requested a review from bhufmann July 4, 2023 15:50
@hoangphamEclipse
Copy link
Contributor Author

@colin-grant-work thank you for your comments. I have updated my PR.

@hoangphamEclipse hoangphamEclipse force-pushed the trace-server-url branch 2 times, most recently from 14d0ce5 to 0c7192f Compare July 11, 2023 15:02
@hoangphamEclipse
Copy link
Contributor Author

Hi @colin-grant-work, thanks for resolving the conversations, if the patch is all good, can you approve? I pushed a rebase patch to fix code conflict.

Copy link

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I have no more objections, but it would be good if another committer could give it a functional test.

@@ -59,6 +60,17 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI {
if (e.affectsConfiguration('trace-compass.traceserver.url') || e.affectsConfiguration('trace-compass.traceserver.apiPath')) {
updateTspClient();
}

if (e.affectsConfiguration('trace-compass.traceserver.url')) {
const newUrl = getTraceServerUrl();

Choose a reason for hiding this comment

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

Investigating remote deployments of this plugin, I've been doing some checking on VSCode utilities for Remote deployments and found the asExternalUri function in the env namespace. Basically, in a desktop deployment, that function does nothing, because external is the same as local, but in a remote deployment, that function transforms the URI into a form that the client can use to access a client running on the host and also does any port forwarding, if the application is set up for that. You can see it in action in the 'Live Preview' plugin, which, like the Trace Extension, opens up a server on the remote machine on certain ports. With that in mind, perhaps the output of getTraceServerUrl should be passed through that utility to ensure that it's accessible.

The caveat that I can see is that if a user is using a VSCode deployment that does not implement the necessary infrastructure, it might be desirable to let the user specify the exact shape of the URL they want the views to use so that the user can set up access to the service manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @colin-grant-work, thank you for the investigation. Are you OK to merge this PR and discuss this later if necessary?

Choose a reason for hiding this comment

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

Yes, that's fine. I'll move this over to an issue.

This commit allow the VSCode extension to listen to the
onDidChangeConfiguration event provided by VSCode and propagate the new
URL to components. The new URL is sent as a message to the front end, then
the TraceServerUrlProvider will run handlers that was registered to the
TSPClientProvider.

Signed-off-by: Hoang Thuan Pham <[email protected]>
@hoangphamEclipse hoangphamEclipse merged commit 9ce89ad into eclipse-cdt-cloud:master Jul 19, 2023
@hoangphamEclipse hoangphamEclipse deleted the trace-server-url branch July 19, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants