[FAI-13683] Add new NodeJs airbyte-local CLI: parsing arguments #94
Conversation
| jest.spyOn(process, 'exit').mockImplementation(() => { | ||
| throw new Error('process.exit() was called by commander js'); | ||
| }); | ||
| const argv = ['./airbyte-local-cli', 'index.js', '--config-file', 'config-file-path', '--src', 'source-image']; |
There was a problem hiding this comment.
Can the executable be set up to not require the index.js argument -- i.e. ./airbyte-local-cli --config-file ... --src ...
There was a problem hiding this comment.
when the user runs the exec in terminal, they don't need index.js.
i need this is because i'm mocking the process argv which i think in node js that's how it works. tbh i'm still a bit loose on this nodejs knowledge so i don't know how this exactly works. https://github.com/tj/commander.js/blob/master/typings/index.d.ts#L727
/**
* Parse `argv`, setting options and invoking commands when defined.
*
* Call with no parameters to parse `process.argv`. Detects Electron and special node options like `node --eval`. Easy mode!
*
* Or call with an array of strings to parse, and optionally where the user arguments start by specifying where the arguments are `from`:
* - `'node'`: default, `argv[0]` is the application and `argv[1]` is the script being run, with user arguments after that
* - `'electron'`: `argv[0]` is the application and `argv[1]` varies depending on whether the electron application is packaged
* - `'user'`: just user arguments
*
* @example
* ```
* await program.parseAsync(); // parse process.argv and auto-detect electron and special node flags
* await program.parseAsync(process.argv); // assume argv[0] is app and argv[1] is script
* await program.parseAsync(my-args, { from: 'user' }); // just user supplied arguments, nothing special about argv[0]
* ```
*
* @returns Promise
*/
| fullRefresh?: boolean | undefined; | ||
| rawMessages?: boolean | undefined; | ||
| keepContainers?: boolean | undefined; | ||
| logLevel?: string | undefined; |
There was a problem hiding this comment.
as there's a few options are affecting each other, and we need to merge and src/dst from 3 ways (file | src/dst option | wizard).
the narrowed down one would be only these.
| ]), | ||
| ) | ||
| .option('--src <image>', 'Airbyte source Docker image') | ||
| .option('--dst <image>', 'Airbyte destination Docker image') |
There was a problem hiding this comment.
didn't set conflict on src and dst as the other two should be sufficient
| .option('--keep-containers', 'Do not remove source and destination containers after they exit') | ||
|
|
||
| // Options: Cli settings | ||
| .option('--debug', 'Enable debug logging') |
There was a problem hiding this comment.
i will create another ticket to revisit this wording.
| return { | ||
| ...options, | ||
| srcImage: options.src, | ||
| dstImage: options.dst, |
There was a problem hiding this comment.
simply rename the image to new name.
src/dst being the image flags are kinda a headache and ambiguous to the src.* / dst.* options
| } | ||
|
|
||
| // Validate the input options | ||
| function validateConfigFileInput(config: FarosConfig, inputType: AirbyteConfigInputType) { |
There was a problem hiding this comment.
uhhh this is kinda ugly. maybe i will think of sth better
| // Take the non-default value if provided with `srcOutputFile` option | ||
| srcOutputFile: cliOptions.srcOnly ? '/dev/null' : cliOptions.srcOutputFile, | ||
| // Rename the `dstOnly` file path to `srcInputFile` | ||
| srcInputFile: cliOptions.dstOnly, |
There was a problem hiding this comment.
or should we rename --dst-only to --src-Input-file? not sure this is a good idea tho
|
|
||
| async function main() { | ||
| logger.debug('Starting Airbyte Local CLI...'); | ||
| await parseAndValidateInputs(process.argv); |
There was a problem hiding this comment.
passing in process.argv explicitly for easier testing.
otherwise commander can implicitly parse this as well.
| "private": true, | ||
| "packageManager": "^npm@10.8.2", | ||
| "scripts": { | ||
| "version": "node -p \"'export const CLI_VERSION = \\'' + require('./package.json').version + '\\';'\" > src/version.ts", |
There was a problem hiding this comment.
any better idea on passing the version to the cli?
| } | ||
| }, | ||
| "catalog": {}, | ||
| "dockerOptions": "--memory 2048m --cpus 2" |
There was a problem hiding this comment.
this is the example of what the file will looks like. lmk your thoughts
| dstUseHostNetwork: cliOptions.dstUseHostNetwork, | ||
| // If `dstOnly` is true, do not pull the source image. Otherwise, fall back to option `noSrcPull` | ||
| srcPull: cliOptions.dstOnly ? false : cliOptions.srcPull, | ||
| dstPull: cliOptions.srcOnly ? false : cliOptions.dstPull, |
There was a problem hiding this comment.
this has to do with commander auto parses --no-src-pull to srcPull instead of noSrcPull.
|
78.9% :(( |
| export interface FarosConfig { | ||
| src?: AirbtyeConfig; | ||
| dst?: AirbtyeConfig; | ||
| srcOutputFile: string | undefined; // if srcOnly is true |
There was a problem hiding this comment.
Can do <name>?: <type> instead of <name>: <type> | undefined for all these properties just like you did for src and dst
There was a problem hiding this comment.
somehow the type complains. but i will look into why
There was a problem hiding this comment.
my understanding is that as the optional fields are from CliOptions, they turned to string | undefined in FarosConfig.
it seems like I will have to do additional checking to avoid the type complaining which i think it's probably not worth it.... :))))) correct me if i'm wrong
I gave all the boolean type fields default values, which i think probably is a better idea anyway.
but keep the string type fields the same.
|
|
79.9%??? ohhhh :(( |
Description
This only has the parsing and validation part of cli.
The options that are being dropped
--src-catalog-overrides <json>--src-catalog-file <path>--src-catalog-json <json>--dst-catalog-file <path>--dst-catalog-json <json>--src-wizard--dst-wizard--dst-stream-prefix <prefix>--max-log-size <size>--max-mem <mem>--max-cpus <cpus>--src-docker-options "<string>--dst-docker-options "<string>--k8s-deployment--k8s-namespace "<string>The options that are renamed:
--check-connection->--src-check-connection--state <path> ->--state-file <path>@cjwooo lmk if you think we shouldn't do any renaming. i just think the new names are more consistent to the rest. or i can keep the old ones as well but have them deprecated
New options
--config-file <path>--wizard <src> [dst]: not determined yet. a placeholder for now.The options stayed
--src <image>--dst <image>--src.<key> <value>--dst.<key> <value>--full-refresh--src-output-file <path>--dst-use-host-network--no-src-pull--no-dst-pull--src-only--dst-only <file>--connection-name--keep-containers--log-level--raw-messages--debugWill address in later PRs
Example of cli output
Type of change