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

Compose: Pass thru list types #64

Closed
wants to merge 3 commits into from

Conversation

plukevdh
Copy link

There's an edge case with Docker Compose command entries that the current code will break.

command: ["bash", "-c", "cmd one ; cmd two"] is supposed to generate a command similar to: bash -c "cmd one ; cmd two".

This ought to generate an ECS task command form of:

"command": [
  "sh",
  "-c",
  "make test ; make run"
]

but instead, it generates

"command": [
  "sh",
  "-c",
  "make",
  "test",
  ";",
  "make",
  "run"
]

Which fails on ECS. This at least fixes the issue I'm seeing, but I'm not entirely aware what else could break.

@emillynge
Copy link

Hi @plukevdh
I'm working on this exact problem as well.
I subitted my own PR before i found yours...
Fix command splitting and joining when an argument contains spaces #65

I think your approach may be flawed. ATM the intermediate format for commands is a string which mirrors native docker (Probably because the docker engine uses an HTTP API). I therefore don't think the answer is to change the internal representation to be a list as you have done in this PR.

Furthermore it doesn't fix the real problem, which is that the method str.split is being used to transform a command str to a list. Rather we should be using the shlex.split method which is made for this purpose specifically.

The current implemetation uses this method:

>>> "echo 'hello world'".split()
['echo', "'hello", "world'"]

Which fails..

@plukevdh
Copy link
Author

As long as we have a version which works as expected with the strings vs. lists, I'm 👌 . This was my intermediate fix which worked for us. If yours does the same (but more consistently) then I'll swap over to use that instead.

@plukevdh
Copy link
Author

Thanks for the feedback and the fix either way!

@emillynge
Copy link

This pr fixes same issue as the merged (https://github.com/micahhausler/container-transform/pull/65)[#65]

@plukevdh plukevdh closed this Jan 2, 2017
@plukevdh plukevdh deleted the nested-commands branch January 2, 2017 15:17
@plukevdh plukevdh restored the nested-commands branch January 2, 2017 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants