-
Notifications
You must be signed in to change notification settings - Fork 206
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
automatic lookup for workspace folder #425
base: main
Are you sure you want to change the base?
Conversation
I think better reopen this. or feel free to reuse my codes. |
@sarashino I just looked at your code and I agree that your Is |
@MunsMan Thanks. P.S. Just for quick reply cause I'm not in front of my desk right now. |
@sarashino Thanks for the prompt Response. I disagree with you about the need of such a feature. I would propose that if the user messes up the working directory, that he should be notified. Otherwise, we might start an unwanted |
@microsoft-github-policy-service agree |
I added this feature to:
Should it be added to |
Had to add default to ensure that `workspace-folder` can't be undefined
Just added a draft for the Furthermore, I wondered if the |
This PR seems to have a great usability value, yet I am not seeing what is blocking this from getting merge, as an interested parties I would like to know and potentially help getting this merged. |
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.
Apologies for not reviewing this PR sooner. Left some comments.
@MunsMan are you still interested in contributing this PR? It would be awesome to merge this in. If so, can you add few tests to validate these changes? Thanks!
if (!(argv['workspace-folder'] || argv['override-config'])) { | ||
throw new Error('Missing required argument: workspace-folder or override-config'); | ||
if (!(argv['id-label'] || argv['override-config'] || isWorkspaceFound)) { | ||
throw new Error('Missing required argument: workspace-folder or id-label or override-config'); |
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.
As workspace-folder
is no longer a required
parameter, this error message could be misleading. Instead can we give a similar error (eg. Dev container config not found.) 👇 if workspace is not found?
cli/src/spec-node/devContainersSpecCLI.ts
Line 424 in efb3ffc
throw new ContainerError({ description: `Dev container config (${uriToFsPath(configFile, cliHost.platform)}) not found.` }); |
if (argv['workspace-folder'] === '.') { | ||
const workspaceFolder = findWorkspaceFolder(); | ||
if (!workspaceFolder) { | ||
throw new Error('Unable to find Configuration File, provide workspace-folder argument'); |
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.
throw new Error('Unable to find Configuration File, provide workspace-folder argument'); | |
throw new Error('Unable to find the dev container config, provide workspace-folder argument'); |
@@ -696,17 +716,23 @@ function runUserCommandsOptions(y: Argv) { | |||
}) | |||
.check(argv => { | |||
const idLabels = (argv['id-label'] && (Array.isArray(argv['id-label']) ? argv['id-label'] : [argv['id-label']])) as string[] | undefined; | |||
const isWorkspaceFound = argv['workspace-folder'] || findWorkspaceFolder(); | |||
console.debug(isWorkspaceFound); |
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.
nit: Should we remove this?
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.
Do you mean the debug log? I just removed it.
The isWorkspaceFound
is required in a lower check, but I adjusted the Error Message there as well.
@samruddhikhandale I will take a look at it over the coming days. The Pull Request is that old, I barely remember the codebase. |
@samruddhikhandale Do you have a proper styling sheet, or a method to style the code before committing? |
@@ -82,6 +83,16 @@ const mountRegex = /^type=(bind|volume),source=([^,]+),target=([^,]+)(?:,externa | |||
|
|||
})().catch(console.error); | |||
|
|||
function findWorkspaceFolder(): 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.
Wouldn't it be sufficient to just make .
the default for --workspace-folder
?
Hey guys, I'm really interested in getting this merged as I think it would greatly improve the CLI. In case @MunsMan has no time to pick this back up I volunteer to help get the feature merged. Can one of the reviewers (@chrmarti, @samruddhikhandale) post a quick reply with the key points required to get this rolling again? |
That would be great. I currently have no use for this and no capacity to write the tests. @kita99 If you can take over, that would be great. |
I'm also willing to finish this up so it can get merged. @chrmarti @samruddhikhandale could one of you please summarize what's missing? It's currently the most up-voted feature request, there's a PR already open and more people willing to contribute. All we need is a reviewer, please. |
Fixes #29 by checking if the current working directory contains a
.devcontainer
folder.I added this check after seeing #332
It looks like that the following two
if
-statements are obsolete.If they are obsolete, just let me know, then I will remove them as well.