fix: sandbox client config options #643
Conversation
🦋 Changeset detectedLatest commit: 79acaf7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -105,7 +105,7 @@ void describe('sandbox command', () => { | |||
| assert.match(output, /--dir-to-watch/); | |||
| assert.match(output, /--exclude/); | |||
| assert.match(output, /--format/); | |||
There was a problem hiding this comment.
Just chatted with the PMs and they suggested changing this to config-format for consistency. If you want to roll that into this PR you could, or do a follow up PR.
There was a problem hiding this comment.
I can add that to this PR. What is your opinion on e2e testing config-format and config-out-dir? I can follow up with another PR when I address #403 or should it be a part of this PR?
There was a problem hiding this comment.
Renamed to config-format in ece9635
There was a problem hiding this comment.
I'm okay with e2e tests going in a follow up PR. When you do get to that, I suggest adding this as an additional assertion on the existing sandbox test rather than a whole new test.
| validateDirectory = (option: string, dir: string) => { | ||
| let stats; | ||
| try { | ||
| stats = fs.statSync(dir, {}); |
There was a problem hiding this comment.
it seems that check might be supporting async functions.
Can you give it a try and convert this to async?
Ref: yargs/yargs#1872
There was a problem hiding this comment.
Was not aware of this, good to know and done in 4cb5b72.
There was a problem hiding this comment.
any file system access if possible should be made async so that event loop is not blocked.
| args['config-out-dir'], | ||
| args['config-format'] |
There was a problem hiding this comment.
can we add a test which asserts that these values are flowing correctly?
| @@ -201,4 +196,16 @@ export class SandboxCommand | |||
| await this.sandboxFactory.getInstance() | |||
| ).delete({ name: this.sandboxName }); | |||
| }; | |||
|
|
|||
| validateDirectory = async (option: string, dir: string) => { | |||
Issue #, if available:
Description of changes:
sandbox --out-diroption tosandbox --config-out-dirconfig-out-dirto client config lifecycle handler so the option is respectedsandbox --formatwas the same and fixed that as wellsandbox --formatoption tosandbox --config-formatamplify sandbox --config-out-dir src --config-format tswill generate the client config in the specifiedsrcdirectory and with the provided format:By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.