-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cli): added build
field to cdk.json
#17176
Changes from 9 commits
f675f56
62b813d
dad551a
52099e1
697b016
f9fda40
7263fc5
ec29717
bee321a
55c348a
6716482
d0691cc
3ec1110
44d49f8
3949ae6
1be1185
d0c0be6
eef80b9
f64725f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,11 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom | |
debug('context:', context); | ||
env[cxapi.CONTEXT_ENV] = JSON.stringify(context); | ||
|
||
const build = config.settings.get(['build']); | ||
if (build) { | ||
await exec(commandToArray(build)); | ||
} | ||
|
||
const app = config.settings.get(['app']); | ||
if (!app) { | ||
throw new Error(`--app is required either in command-line, in ${PROJECT_CONFIG} or in ${USER_DEFAULTS}`); | ||
|
@@ -57,7 +62,7 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom | |
return createAssembly(app); | ||
} | ||
|
||
const commandLine = await guessExecutable(appToArray(app)); | ||
const commandLine = await guessExecutable(commandToArray(app)); | ||
|
||
const outdir = config.settings.get(['output']); | ||
if (!outdir) { | ||
|
@@ -74,7 +79,7 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom | |
|
||
debug('env:', env); | ||
|
||
await exec(); | ||
await exec(commandLine); | ||
|
||
return createAssembly(outdir); | ||
|
||
|
@@ -91,7 +96,7 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom | |
} | ||
} | ||
|
||
async function exec() { | ||
async function exec(commandAndArgs: string[]) { | ||
return new Promise<void>((ok, fail) => { | ||
// We use a slightly lower-level interface to: | ||
// | ||
|
@@ -103,7 +108,9 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom | |
// anyway, and if the subprocess is printing to it for debugging purposes the | ||
// user gets to see it sooner. Plus, capturing doesn't interact nicely with some | ||
// processes like Maven. | ||
const proc = childProcess.spawn(commandLine[0], commandLine.slice(1), { | ||
const command = commandAndArgs[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we just need to accept a single string and pass it down to spawn(). Since you use shell:true this should just work without splitting to arguments imho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to split the arguments because of the other usage of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eladb if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems a bit messy but if this works as is i am okay with that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eladb just made this cleaner, now |
||
const args = commandAndArgs.slice(1); | ||
const proc = childProcess.spawn(command, args, { | ||
comcalvi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
stdio: ['ignore', 'inherit', 'inherit'], | ||
detached: false, | ||
shell: true, | ||
|
@@ -150,12 +157,12 @@ async function populateDefaultEnvironmentIfNeeded(aws: SdkProvider, env: { [key: | |
} | ||
|
||
/** | ||
* Make sure the 'app' is an array | ||
* Make sure the 'command' is an array | ||
* | ||
* If it's a string, split on spaces as a trivial way of tokenizing the command line. | ||
*/ | ||
function appToArray(app: any) { | ||
return typeof app === 'string' ? app.split(' ') : app; | ||
function commandToArray(command: any) { | ||
return typeof command === 'string' ? command.split(' ') : command; | ||
} | ||
|
||
type CommandGenerator = (file: string) => string[]; | ||
|
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.
Not sure this is better TBH...
What happens if the command has quotes in it?
For example
"mvn package"
?Bottom line, I think we should simply spawn this command without splitting it into arguments and with
shell:true
. There's a variant of spawn I believe that just accepts a single string and passes it to the shell.There are also intricacies related to Windows/POSIX here that can blow up in 5,000 ways, so I rather we avoid any parsing of the command line if possible.
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.
@eladb just to be clear - you also want to change how we handle the
"app"
key, right? And no longer do any splitting there?