-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix: improve openDevContainer support with local workspace folder
#544
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
localWorkspaceFolderandlocalConfigFilequery parameters inextension.ts - Extend
Commandsto forward new parameters and adjustopenDevContainerbehavior - Update
CHANGELOG.mdto 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
localWorkspaceFolderandlocalConfigFileto ensure correct authority strings and request payloads.
async function openDevContainer(
aslilac
left a comment
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
src/commands.ts
Outdated
| 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.
src/commands.ts
Outdated
| ).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
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.
Haha, I fixed this locally but it didn't get pushed. I know it doesn't matter in the context, but it just feels better you know, some habits die hard. 😄
|
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 |
f79393a to
086ec55
Compare
|
@aslilac made the proposed changes 👍🏻 |
Understandable. 👍🏻 @DanielleMaywood researched this topic in the past and came to the conclusion that These APIs are totally undocumented and have been reverse-engineered. With some additional effort we discovered that More context at: coder/coder#16426 (comment) |
openDevContainer support with local workspace folder
DanielleMaywood
left a comment
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.
Works on my machine ™️, LGTM!
aslilac
left a comment
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 for the context 😄
This change allows
localWorkspaceFolderandlocalConfigFileto beprovided which allows us to use
dev-containerrather thanattached-containerto enter enabling all dev container features likefile change detection and rebuilding.
Example open URL: