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

Fix command splitting and joining when an argument contains spaces #65

Merged
merged 1 commit into from
Jan 2, 2017

Conversation

emillynge
Copy link

At the moment ECS and compose transformers performs a "dumb" argument splitting.

This means that the following compose entrypoint:

entrypoint: ['/bin/echo', 'Hello world']

gets incorrectly transformed into /bin/echo hello world which fails as echo only expects 1 argument.

conversely ECS emits '/bin/echo "Hello world" as

"entryPoint": [
               "/bin/echo",
               "\"Hello",
               "world\""
           ]

This PR fixes these and adds tests for the failure mode.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling be510cc on emillynge:shlex_splitting into ** on micahhausler:master**.

@micahhausler
Copy link
Owner

@emillynge thanks for making this! Any chance you could squash the 3 commits down to one?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0cd967e on emillynge:shlex_splitting into ** on micahhausler:master**.

@emillynge
Copy link
Author

I would, but it seems like a hassle that is unnecessary since this is actually built-in as a tool for the maintainer.

Squash to only one “proper” commit for github pull request

Note that Bob doesn't have to squash his commits when he is making a GitHub PR.
Since March 2016, you can leave that operation to the maintainer (you) accepting your PR.

The thread refers to this post Squash your commits

@emillynge
Copy link
Author

emillynge commented Dec 21, 2016

By the way, I have expanded the code change to the kubernetes, marathon and chronos module as well for consistency.
I don't use these, so please check that I haven't fucked something up.

Also I have changed the "unsplitting" slightly as I found out that shlex.quote() is a bit agressive (quoting stuff that did not need escaping).
Now I use shlex.split to check whether quoting is necessary for each item in a list of commands. To have this, slightly more complex, code declared only once, I have taken the liberty of adding a static method ._list2cmdline(commands: list) -> str to the Transformer base class. (I couldn't find any utils module)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 472f9b6 on emillynge:shlex_splitting into ** on micahhausler:master**.

@emillynge
Copy link
Author

Found the time to squash to just 1 commit. :)

Should be ready to merge without further ado

@emillynge
Copy link
Author

I'm thinking @micahhausler maybe didn't get a notification :)

Everything should be ready to merge.

@micahhausler
Copy link
Owner

Thanks @emillynge !

@micahhausler micahhausler merged commit f12926d into micahhausler:master Jan 2, 2017
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.

None yet

3 participants