Skip to content

Bugfig, allowing all paths to have spaces #244

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

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

AndrewFarley
Copy link
Contributor

@AndrewFarley AndrewFarley commented Sep 14, 2018

Fixes #242

Problem

Could not use this plugin with any path that contains a space character due to missing/insufficient shell escaping/quoting.

Solution

I'm sorry I couldn't find a simple "quote-all" solution similar to what was in place pre-merge. These solutions fall apart because certain things you can't quote or docker freaks out, for example features like the 'slim' would fall apart... eg: (see the last line)

   [ '-m',
     'pip',
     'install',
     '-t',
     '\'/root/test me/serverless-python-requirements-farley/tests/base/.serverless/requirements\'',
     '-r',
     '\'/root/test me/serverless-python-requirements-farley/tests/base/.serverless/requirements/requirements.txt\'',
     '" && find /root/test me/serverless-python-requirements-farley/tests/base/.serverless/requirements -name \\"*.so\\" -exec strip {} \';\'"' ]

The "simple" single test you added for testing in a subfolder only tests one of many use-cases that use various inputs for volumes, caching, etc. This MR I tested (as you see above) running ALL tests from a folder with a space in it, confirmed working. Just, not as clean as I would like, but I spent 4 hours trying to make it work with a quote all args type mechanism. If you don't like this MR's style, please let me know, but I'm out of effort/time/etc. I can post my diff for my attempts as using a simpler quote-all if interested. I did not, however, go test if this breaks anything on Windows. :\ We really need automated testing on Windows somehow @dschep.

@AndrewFarley
Copy link
Contributor Author

Ah crap, idk what is up with that failing test, but I'll look into it later this evening. :P

@dschep
Copy link
Contributor

dschep commented Sep 14, 2018

The error was docker not found which I assumed was a CircleCI issue, and upon retrying, tests pass, so it was!

@dschep dschep merged commit 8a32692 into serverless:master Sep 14, 2018
@AndrewFarley AndrewFarley deleted the bug-space-paths branch February 28, 2020 04:06
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