Skip to content

fix: improve /openDevContainer support with local workspace folder #544

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Jul 1, 2025

This change allows localWorkspaceFolder and localConfigFile to be
provided which allows us to use dev-container rather than
attached-container to enter enabling all dev container features like
file change detection and rebuilding.

Example open URL:

vscode://coder.coder-remote/openDevContainer?owner=mafredri&workspace=magenta-rat-8&agent=dev&devContainerName=boring_shaw&devContainerFolder=/workspaces/coder&localWorkspaceFolder=/home/coder/coder&localConfigFile=/home/coder/coder/.devcontainer/devcontainer.json

This change allows `localWorkspaceFolder` and `localConfigFile` to be
provided which allows us to use `dev-container` rather than
`attached-container` to enter enabling all dev container features like
file change detection and rebuilding.
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 enhances the /openDevContainer command to accept local paths, enabling full dev-container features when a local workspace folder and config file are provided.

  • Add localWorkspaceFolder and localConfigFile query parameters in extension.ts
  • Extend Commands to forward new parameters and adjust openDevContainer behavior
  • Update CHANGELOG.md to document the new support

Reviewed Changes

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

File Description
src/extension.ts Read, validate, and forward localWorkspaceFolder & localConfigFile
src/commands.ts Accept new CLI args, include hostPath/configFile, choose container type
CHANGELOG.md Document unreleased support for hostPath and configFile
Comments suppressed due to low confidence (1)

src/commands.ts:751

  • Add unit or integration tests covering scenarios with and without localWorkspaceFolder and localConfigFile to ensure correct authority strings and request payloads.
async function openDevContainer(

@mafredri mafredri requested review from aslilac and code-asher July 1, 2025 16:40
mafredri added a commit to coder/coder that referenced this pull request Jul 1, 2025
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

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

I really don't know enough about dev containers or this extensions code to feel comfortable approving, but I did have a couple notes

Comment on lines +634 to +635
const localWorkspaceFolder = args[5] as string | undefined;
const localConfigFile = args[6] as string | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't a new thing you're doing but ???

why does this function not just take named arguments??? I really wish someone had left a justification in a comment.

"utf-8",
).toString("hex");
const devContainerAuthority = `attached-container+${devContainer}@${remoteAuthority}`;

const type = !!localWorkspaceFolder ? "dev-container" : "attached-container";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const type = !!localWorkspaceFolder ? "dev-container" : "attached-container";
const type = localWorkspaceFolder ? "dev-container" : "attached-container";

as the linter is saying, this does nothing

@aslilac
Copy link
Member

aslilac commented Jul 1, 2025

tested locally and this diff seems to work fine. could we sneak it in here?

diff --git a/src/commands.ts b/src/commands.ts
index c1d49f9..375bca4 100644
--- a/src/commands.ts
+++ b/src/commands.ts
@@ -620,18 +620,18 @@ export class Commands {
 	 *
 	 * Throw if not logged into a deployment.
 	 */
-	public async openDevContainer(...args: string[]): Promise<void> {
+	public async openDevContainer(
+		workspaceOwner: string,
+		workspaceName: string,
+		workspaceAgent: string,
+		devContainerName: string,
+		devContainerFolder: string,
+	): Promise<void> {
 		const baseUrl = this.restClient.getAxiosInstance().defaults.baseURL;
 		if (!baseUrl) {
 			throw new Error("You are not logged in");
 		}
 
-		const workspaceOwner = args[0] as string;
-		const workspaceName = args[1] as string;
-		const workspaceAgent = args[2] as string;
-		const devContainerName = args[3] as string;
-		const devContainerFolder = args[4] as string;
-
 		await openDevContainer(
 			baseUrl,
 			workspaceOwner,

you can add your new arguments to the end as optional

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