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

use current directory as default workspace-folder #387

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sarashino
Copy link

@sarashino sarashino requested a review from a team as a code owner January 28, 2023 16:51
@sarashino
Copy link
Author

@microsoft-github-policy-service agree

@sarashino
Copy link
Author

#29

@samruddhikhandale
Copy link
Member

Looks like this merged PR already updated the commands ? --> https://github.com/devcontainers/cli/pull/378/files/5793e9eda8a852a7a94a7c96215bc1ab7c200586#diff-eb1f4feeeea826490afd8c5331c930fa33c019a97ffc6ae6b42b6dd95acf7803L513

Seems like it's released in v0.29.0 ?

@sarashino sarashino closed this Feb 7, 2023
@sarashino sarashino reopened this Feb 28, 2023
MunsMan added a commit to MunsMan/devcontainercli that referenced this pull request Feb 28, 2023
@aedrax
Copy link

aedrax commented Aug 9, 2023

I hope they merge this in at some point

@hpe-ykoehler
Copy link

Any chance we could get someone to review / comment to get this in? Nearly a year now...

@samruddhikhandale
Copy link
Member

This one definitely slipped through the gaps ; apologies for the delay, and thanks for your patience.

Looping in @chrmarti for an extra set of eyes.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Thank you so much for contributing this PR and we really apologize to miss such a quality update! 😿

curious, @sarashino are you still interested in contributing to it? That would be wonderful. Thank you!

@@ -90,6 +91,24 @@ const mountRegex = /^type=(bind|volume),source=([^,]+),target=([^,]+)(?:,externa

export type UnpackArgv<T> = T extends Argv<infer U> ? U : T;

function findWorkspaceFolder(): 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.

Can we add some tests to validate these changes?

let ceilingDirsEnv = process.env.DEVCONTAINERS_CEILING_DIRECTORIES;
const ceilingDirs = ceilingDirsEnv ? ceilingDirsEnv.split(':') : [];
while (true){
if(fs.existsSync(target + '/.devcontainer')){
Copy link
Member

Choose a reason for hiding this comment

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

Should we also look for the.devcontainer.json file? It might be present without the .devcontainer folder.

if (!(argv['workspace-folder'] || argv['override-config'])) {
throw new Error('Missing required argument: workspace-folder or override-config');
if (!(isWorkspaceFound || argv['id-label'] || argv['override-config'])) {
throw new Error('Missing required argument: workspace-folder, id-label, or override-config');
Copy link
Member

Choose a reason for hiding this comment

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

As this PR automatically computes workspace-folder, I wonder if we need to remove workspace-folder from the requires args list 🤔

If so, we'd need to update the error message accordingly.

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.

4 participants