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

Dev Container uses the right workspace now #14557

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Conversation

jbicker
Copy link
Contributor

@jbicker jbicker commented Nov 28, 2024

What it does

When there are more than one workspaces open the config json is picked from the right workspace.
Solves #14307

How to test

See #14307 and try to reproduce the issue. Should not be possible anymore. :-)

Review checklist

Reminder for reviewers

When there are more than one workspaces open the config json is picked from the right workspace.
Solves #14307
@jbicker jbicker requested a review from jonah-iden November 28, 2024 16:12
@jonah-iden
Copy link
Contributor

The linting task still fails

Copy link
Contributor

@jonah-iden jonah-iden left a comment

Choose a reason for hiding this comment

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

For documentation purposes: There is no support for multi root workspaces yet. But thats something for another task
Works like a charm, and code looks good.

@jbicker jbicker merged commit 689bd51 into master Nov 29, 2024
11 checks passed
@jbicker jbicker deleted the jbicker/issue-14307 branch November 29, 2024 10:16
@github-actions github-actions bot added this to the 1.57.0 milestone Nov 29, 2024
@@ -54,7 +49,7 @@ export class DevContainerFileService {
}

protected async searchForDevontainerJsonFiles(directory: string, depth: number): Promise<string[]> {
if (depth < 0) {
if (depth < 0 || !fs.existsSync(directory)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use sync file system code in the backend - it blocks the whole server process and is bad practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry overlooked that. PR here #14563

@jfaltermeier
Copy link
Contributor

Just an FYI, this doesn’t seem to work on the Theia IDE Preview for 1.57 at the moment. We haven’t investigated yet: #14643 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants