-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: main
Are you sure you want to change the base?
Conversation
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.
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.
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
andlocalConfigFile
query parameters inextension.ts
- Extend
Commands
to forward new parameters and adjustopenDevContainer
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
andlocalConfigFile
to ensure correct authority strings and request payloads.
async function openDevContainer(
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.
I really don't know enough about dev containers or this extensions code to feel comfortable approving, but I did have a couple notes
const localWorkspaceFolder = args[5] as string | undefined; | ||
const localConfigFile = args[6] as string | undefined; |
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.
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"; |
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.
const type = !!localWorkspaceFolder ? "dev-container" : "attached-container"; | |
const type = localWorkspaceFolder ? "dev-container" : "attached-container"; |
as the linter is saying, this does nothing
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 |
This change allows
localWorkspaceFolder
andlocalConfigFile
to beprovided which allows us to use
dev-container
rather thanattached-container
to enter enabling all dev container features likefile change detection and rebuilding.
Example open URL: