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

fix: sandbox client config options #643

Merged
merged 8 commits into from Nov 15, 2023
5 changes: 5 additions & 0 deletions .changeset/dry-ladybugs-film.md
@@ -0,0 +1,5 @@
---
'@aws-amplify/backend-cli': patch
---

client config generation respects sandbox format option
5 changes: 5 additions & 0 deletions .changeset/serious-apricots-love.md
@@ -0,0 +1,5 @@
---
'@aws-amplify/backend-cli': patch
---

Rename sandbox format to config-format
5 changes: 5 additions & 0 deletions .changeset/strong-plums-explain.md
@@ -0,0 +1,5 @@
---
'@aws-amplify/backend-cli': patch
---

Change sandbox out-dir to config-out-dir and client config generation respects config-out-dir option
36 changes: 34 additions & 2 deletions packages/cli/src/commands/sandbox/sandbox_command.test.ts
Expand Up @@ -104,8 +104,8 @@ void describe('sandbox command', () => {
assert.match(output, /--name/);
assert.match(output, /--dir-to-watch/);
assert.match(output, /--exclude/);
assert.match(output, /--format/);
assert.match(output, /--out-dir/);
assert.match(output, /--config-format/);
assert.match(output, /--config-out-dir/);
assert.equal(mockHandleProfile.mock.callCount(), 0);
});

Expand Down Expand Up @@ -251,4 +251,36 @@ void describe('sandbox command', () => {
sandboxProfile
);
});

void it('fails if invalid config-out-dir is provided', async () => {
mock.method(fs, 'lstatSync', () => {
return {
isFile: () => false,
isDir: () => false,
};
});
const output = await commandRunner.runCommand(
'sandbox --config-out-dir nonExistentDir'
);
assert.match(output, /--config-out-dir nonExistentDir does not exist/);
});

void it('fails if a file is provided for config-out-dir', async (contextual) => {
mock.method(fs, 'lstatSync', () => {
return {
isFile: () => true,
isDir: () => false,
};
});
contextual.mock.method(fs, 'statSync', () => ({
isDirectory: () => false,
}));
const output = await commandRunner.runCommand(
'sandbox --config-out-dir existentFile'
);
assert.match(
output,
/--config-out-dir existentFile is not a valid directory/
);
});
});
46 changes: 25 additions & 21 deletions packages/cli/src/commands/sandbox/sandbox_command.ts
Expand Up @@ -19,8 +19,8 @@ type SandboxCommandOptionsCamelCase = {
dirToWatch: string | undefined;
exclude: string[] | undefined;
name: string | undefined;
format: ClientConfigFormat | undefined;
outDir: string | undefined;
configFormat: ClientConfigFormat | undefined;
configOutDir: string | undefined;
profile: string | undefined;
};

Expand Down Expand Up @@ -81,7 +81,9 @@ export class SandboxCommand

// attaching event handlers
const clientConfigLifecycleHandler = new ClientConfigLifecycleHandler(
this.clientConfigGeneratorAdapter
this.clientConfigGeneratorAdapter,
args['config-out-dir'],
args['config-format']
);
const eventHandlers = this.sandboxEventHandlerCreator?.({
sandboxName: this.sandboxName,
Expand All @@ -94,8 +96,8 @@ export class SandboxCommand
}
const watchExclusions = args.exclude ?? [];
const clientConfigWritePath = await getClientConfigPath(
args['out-dir'],
args.format
args['config-out-dir'],
args['config-format']
Comment on lines +99 to +100
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 a test which asserts that these values are flowing correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 70969c4

);
watchExclusions.push(clientConfigWritePath);
await sandbox.start({
Expand Down Expand Up @@ -136,14 +138,14 @@ export class SandboxCommand
array: false,
global: false,
})
.option('format', {
.option('config-format', {
describe: 'Client config output format',
type: 'string',
array: false,
choices: Object.values(ClientConfigFormat),
global: false,
})
.option('out-dir', {
.option('config-out-dir', {
describe:
'A path to directory where config is written. If not provided defaults to current process working directory.',
type: 'string',
Expand All @@ -157,20 +159,10 @@ export class SandboxCommand
})
.check((argv) => {
if (argv['dir-to-watch']) {
// make sure it's a real directory
let stats;
try {
stats = fs.statSync(argv['dir-to-watch'], {});
} catch (e) {
throw new Error(
`--dir-to-watch ${argv['dir-to-watch']} does not exist`
);
}
if (!stats.isDirectory()) {
throw new Error(
`--dir-to-watch ${argv['dir-to-watch']} is not a valid directory`
);
}
this.validateDirectory('dir-to-watch', argv['dir-to-watch']);
}
if (argv['config-out-dir']) {
this.validateDirectory('config-out-dir', argv['config-out-dir']);
}
if (argv.name) {
const projectNameRegex = /^[a-zA-Z0-9-]{1,15}$/;
Expand Down Expand Up @@ -201,4 +193,16 @@ export class SandboxCommand
await this.sandboxFactory.getInstance()
).delete({ name: this.sandboxName });
};

validateDirectory = (option: string, dir: string) => {
let stats;
try {
stats = fs.statSync(dir, {});
Copy link
Member

Choose a reason for hiding this comment

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

it seems that check might be supporting async functions.
Can you give it a try and convert this to async?

Ref: yargs/yargs#1872

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was not aware of this, good to know and done in 4cb5b72.

Copy link
Member

Choose a reason for hiding this comment

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

any file system access if possible should be made async so that event loop is not blocked.

} catch (e) {
throw new Error(`--${option} ${dir} does not exist`);
}
if (!stats.isDirectory()) {
throw new Error(`--${option} ${dir} is not a valid directory`);
}
};
}