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

feat: add ability to attach to devcontainers #463

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

DanielleMaywood
Copy link
Collaborator

@DanielleMaywood DanielleMaywood commented Apr 1, 2025

Relates to coder/coder#16426

Adds the ability to connect to a running devcontainer inside a Coder Workspace.

We add a new handler to handle the URI

vscode://coder.coder-remote/openDevContainer

With the following query params:

  • url: The URL of the deployment
  • token: The API key
  • owner: The workspace owner
  • workspace: The workspace name
  • devContainerName: The name of the dev container
  • devContainerFolder: The folder inside the dev container

Adds the ability to connect to a running devcontainer inside a Coder
Worksapce.
@DanielleMaywood DanielleMaywood requested a review from Copilot April 1, 2025 09:47
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds functionality to attach to a running devcontainer within a Coder Workspace. The changes include introducing a new utility to build remote authority strings, updating the extension to handle a new "/openDevContainer" path and register a corresponding command, and implementing the openDevContainer command in the commands module.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/util.ts Adds toRemoteAuthority to centralize remote authority string logic.
src/extension.ts Introduces new query parameter processing and command invocation for devcontainers.
src/commands.ts Implements openDevContainer command and refactors openWorkspace to leverage toRemoteAuthority.

Comment on lines +515 to +516
const workspaceAgent = undefined // args[2] is reserved, but we do not support multiple agents yet.
const devContainerName = args[3] as string
Copy link
Preview

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

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

The extension passes a workspaceAgent parameter for the openDevContainer command, but this parameter is being discarded by explicitly setting workspaceAgent to undefined. Consider either using args[2] if provided or updating the extension to no longer supply the parameter.

Suggested change
const workspaceAgent = undefined // args[2] is reserved, but we do not support multiple agents yet.
const devContainerName = args[3] as string
const workspaceAgent = args[2] as WorkspaceAgent | undefined // Use args[2] if provided.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already have precedent for doing this with the openWorkspace flow

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Slick! I only tested VS Code (not Cursor, etc), but it looks good.

const devContainerFolder = params.get("devContainerFolder")

if (!workspaceOwner) {
throw new Error("workspace owner must be specified as a query parameter")
Copy link
Member

Choose a reason for hiding this comment

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

No need to change in this PR since this is a pattern that already existed, but at some point I wonder if we should catch handler errors and show them in an error notification. Currently the only way to see them seems to be to go to the extension's features > runtime status > uncaught errors.

@DanielleMaywood DanielleMaywood merged commit 1922834 into main Apr 2, 2025
2 checks passed
@DanielleMaywood DanielleMaywood deleted the dm-devcontainers branch April 2, 2025 08:21
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.

2 participants