Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #17176feat(cli): added
build
field to cdk.json #17176Changes from 4 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
There are no files selected for viewing
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.
What happens if "build" is set to something like "mvn package"? How does this split into program and args?
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.
exec()
now splits its argument intocommand
andargs
before spawning the process.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.
Seems like exec() treats the first item in the array as the command but this is not true in the case I described above.
Can you add a test to verify ?
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.
You're right, the split doesn't happen in the case of the
build
key. It winds up working because the whole command just get put intocommand
andspawn()
happily accepts it.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.
What happens if
commandAndArgs[0]
ismvn package
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
We need to split the arguments because of the other usage of
exec()
, namely this line:await exec(commandLine);
commandLine
is defined byconst commandLine = await guessExecutable(appToArray(app));
guessExecutable()
needs astring[]
, not astring
. Do you want me to rework this so that we don't need to have anystring[]
s passed toexec()
, and instead makeexec()
take just astring
?exec()
previously operated on astring[]
.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 if
commandAndArgs[0]
ismvn package
thenspawn()
will still correctly start the process.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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@eladb just made this cleaner, now
mvn package
will be split into["mvn", "package"]
instead of the previous["mvn package"]
. Would appreciate another pass on the PR!