feat: Uses cwd as default workspace folder #956
Conversation
|
@microsoft-github-policy-service agree |
|
I agree that this would make life easier and I don't understand why it wasn't by default |
| @@ -507,7 +497,7 @@ function buildOptions(y: Argv) { | |||
| 'user-data-folder': { type: 'string', description: 'Host path to a directory that is intended to be persisted and share state between sessions.' }, | |||
| 'docker-path': { type: 'string', description: 'Docker CLI path.' }, | |||
| 'docker-compose-path': { type: 'string', description: 'Docker Compose CLI path.' }, | |||
| 'workspace-folder': { type: 'string', required: true, description: 'Workspace folder path. The devcontainer.json will be looked up relative to this path.' }, | |||
| 'workspace-folder': { type: 'string', description: 'Workspace folder path. The devcontainer.json will be looked up relative to this path.' }, | |||
There was a problem hiding this comment.
Mention that the current working directory is used by default. (Also in a few other places in this file.)
There was a problem hiding this comment.
Yes. I actually just moved the default to the yargs entirely, so no need to manually handle null.
ekwoka
force-pushed
the
default-workspace-folder
branch
from
292e231 to
4c2a428
Compare
And fixes broken test syntaxes
|
Guess I got a style thing to fix ☢️ |
|
CI doesn't seem to pick up the latest commit. Will close and reopen to trigger fresh. |
There was a problem hiding this comment.
Just realized that run-user-commands, read-configuration and exec can be run without a workspace folder. This is to support attaching to an existing container that was not created with up. Making the cwd the workspace folder breaks this case.
For these commands maybe the cwd can be the default when none of --container-id, --id-label or --workspace-folder are given.
What do you think?
| @@ -752,7 +746,7 @@ function runUserCommandsOptions(y: Argv) { | |||
| 'docker-compose-path': { type: 'string', description: 'Docker Compose CLI path.' }, | |||
| 'container-data-folder': { type: 'string', description: 'Container data folder where user data inside the container will be stored.' }, | |||
| 'container-system-data-folder': { type: 'string', description: 'Container system data folder where system data inside the container will be stored.' }, | |||
| 'workspace-folder': { type: 'string', description: 'Workspace folder path. The devcontainer.json will be looked up relative to this path.' }, | |||
| 'workspace-folder': { type: 'string', description: 'Workspace folder path. The devcontainer.json will be looked up relative to this path.', default: '.', defaultDescription: 'Current Working Directory' }, | |||
There was a problem hiding this comment.
Actually, run-user-commands can be run without a workspace folder when attaching to an existing container that is not based on a workspace with a devcontainer.json.
For compatibility it might make sense to keep the existing behavior.
There was a problem hiding this comment.
What is the behavior meant to be for when a container ID AND workspace folder are provided right now?
Would this impact them?
If they pass the tests still, does this indicate the tests for container-id (and others) alone are incomplete?
Does the path not already prefer those before the workspace folder?
There was a problem hiding this comment.
When container id and workspace folder are given the container is looked up based on the container id. I think that was faster than the lookup by label.
The tests are incomplete, additions appreciated.
For consistency this change could assume the cwd as the workspace folder in all commands only when no container id and no id labels are given. Conveniently this case currently fails, so the change wouldn't break existing working setups. This should still cover most cases when run from a terminal because these rarely use the container id or id labels - I expect. Does this make sense?
| @@ -974,9 +965,6 @@ function readConfigurationOptions(y: Argv) { | |||
| if (idLabels?.some(idLabel => !/.+=.+/.test(idLabel))) { | |||
| throw new Error('Unmatched argument format: id-label must match <name>=<value>'); | |||
| } | |||
| if (!argv['container-id'] && !idLabels?.length && !argv['workspace-folder']) { | |||
There was a problem hiding this comment.
Same as with run-user-commands, read-configuration can be run without workspace folder.
| @@ -1242,9 +1230,6 @@ function execOptions(y: Argv) { | |||
| if (remoteEnvs?.some(remoteEnv => !/.+=.*/.test(remoteEnv))) { | |||
| throw new Error('Unmatched argument format: remote-env must match <name>=<value>'); | |||
| } | |||
| if (!argv['container-id'] && !idLabels?.length && !argv['workspace-folder']) { | |||
This basically makes the equivalent of
--workspace-folder .the default.I'd imagine that this is 99% of use cases already, and those that need something else will know they need something else.
I think I covered all the commands this would apply to with the tests.