Split mixin command args by whitespace #931
Conversation
|
/azp run porter |
|
/azp run porter-integration |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
pkg/exec/builder/execute.go
Outdated
| // Split up any arguments or flags that have spaces so that we pass them as separate array elements | ||
| // It doesn't show up any differently in the printed command, but it matters to how the command | ||
| // it executed against the system. | ||
| args = expandOnWhitespace(args) |
There was a problem hiding this comment.
Do you think passing the command off to the shell (which parses this for us) be something that would work here instead of parsing the command ourselves? Looks similar to the problem I ran into here: https://github.com/deislabs/porter/pull/921/files#diff-9cde54dd12e8d18ebea48c996aa54c92R45
There was a problem hiding this comment.
@halkyon Do you have a cross platform way of doing that? I was looking for ideas yesterday on how to make the shell deal with this instead of that kludge function (that doesn't work anyway). I would love some help fixing this problem! 😂
In some invocation images, there isn't a shell, they may have built the image on something without bash. That's why I ask.
There was a problem hiding this comment.
It is so tempting to be like bash -c "dump all their stuff here" but actually their command may also be a bash -c "..." command too. Or it could be calling to a binary that is in a scratch based invocation image.
There was a problem hiding this comment.
Ah, right, I didn’t realise the environments commands could be running in, still getting my head around porter. Sorry for butting in a bit on this PR by the way!
There was a problem hiding this comment.
Oh hey no worries, I liked the suggestion and would have used it in a heartbeat if I could!
carolynvs
force-pushed
the
mixin-arg-parsing
branch
from
3e66098 to
4019357
Compare
Some mixin commands have whitespace and right now we are passing them in
as packed into a single array element when executing the command
exec.Cmd("aws", []string{"ec2", "run-instances", "--security-group-ids
group1 group2"})
When the debug command is printed, we can't tell that this has happened,
and why the command failed. What needs to happen is that when there is a
space, and it's not enclosed in double quotes, it should be split into
its own array element
exec.Cmd("aws", []string{"ec2", "run-instances", "--security-group-ids",
"group1", "group2"})
carolynvs
force-pushed
the
mixin-arg-parsing
branch
from
4019357 to
0b4a4bb
Compare
|
I don't think I'm going to get away with my cheap trick with the csv parser. 😉 I just added a failing test case for enclosing escaped double quotes and will work on writing an artisanal small batch parser for our own needs. |
* Split by space * Keep single and double quoted text together * Ignore escaped quotes * When quotes don't match, preserve original string
carolynvs
force-pushed
the
mixin-arg-parsing
branch
from
fe25930 to
d8bea60
Compare
|
Okay, artisanal small batch parser works. I want to hold off on merging this until I get some more pieces in place however since this is a disruptive change. It removes a bug that people relied upon that allowed them to call Here are some things that need to be done before we do a release:
We don't need all of these done before this can be merged, I just want to get a plan in place to do this in a timely fashion so that we don't have canary borked for a while. |
Do not assume that mixin arguments and flags should be passed as a quoted string to the cli. Require that they are wrapped in quotes instead and otherwise split on space to pass them separately. See getporter/porter#931 for how this works.
Do not assume that mixin arguments and flags should be passed as a quoted string to the cli. Require that they are wrapped in quotes instead and otherwise split on space to pass them separately. See getporter/porter#931 for how this works.
Do not assume that mixin arguments and flags should be passed as a quoted string to the cli. Require that they are wrapped in quotes instead and otherwise split on space to pass them separately. See getporter/porter#931 for how this works.
Do not assume that mixin arguments and flags should be passed as a quoted string to the cli. Require that they are wrapped in quotes instead and otherwise split on space to pass them separately. See getporter/porter#931 for how this works.
Do not assume that mixin arguments and flags should be passed as a quoted string to the cli. Require that they are wrapped in quotes instead and otherwise split on space to pass them separately. See getporter/porter#931 for how this works.
What does this change
Some mixin commands have whitespace and right now we are passing them in as packed into a single array element when executing the command
exec.Cmd("aws", []string{"ec2", "run-instances", "--security-group-ids group1 group2"})When the debug command is printed, we can't tell that this has happened, and why the command failed. What needs to happen is that when there is a space, and it's not enclosed in double quotes, it should be split into its own array element
exec.Cmd("aws", []string{"ec2", "run-instances", "--security-group-ids", "group1", "group2"})What issue does it fix
I think this will help with getporter/aws-mixin#15
Here is how it would work
If you really needed the flag to get both arguments passed together then you would use this:
Notes for the reviewer
Since the command looks the same when printed, I wasn't sure how to test this out other than just testing the function I added.
Checklist