fix(path): change windows style path to unix #633
Conversation
samcli/local/lambdafn/utils.py
Outdated
|
|
||
| def path_modifier(func): | ||
| """ | ||
| This method decorator allows conversions of windows style path to unix style path |
There was a problem hiding this comment.
Can you update this to use numpy doc strings instead? We are moving off this format onto the numpy format with new docs.
|
|
||
| os_mock.name = "nt" | ||
| with mock.patch("os.path", ntpath): | ||
| ntpath.isabs = lambda path: False |
There was a problem hiding this comment.
ooo... this is interesting, never considered this a way to mock functions out before!
| def test_must_resolve_windows_path_to_unix_paths(self, os_mock): | ||
|
|
||
| os_mock.name = "nt" | ||
| with mock.patch("os.path", ntpath): |
There was a problem hiding this comment.
This could just be a style difference but why would you do this over mocking out os.path through os_mock.path = mock()?
jfuss
added
stage/in-progress
sriram-mv
force-pushed
the
windows_style_playground
branch
3 times, most recently
from
71011a9
to
07cce2d
Compare
samcli/local/docker/container.py
Outdated
| @@ -53,7 +54,7 @@ def __init__(self, | |||
| self._image = image | |||
| self._cmd = cmd | |||
| self._working_dir = working_dir | |||
| self._host_dir = host_dir | |||
| self._host_dir = posix_path(host_dir) | |||
There was a problem hiding this comment.
Can we set the posix_path on all the volumes that get mounted? Apply to the kwargs on the create() method.
When we mount other volumes in the future, this will just transparently work.
samcli/local/docker/utils.py
Outdated
| code_path : str | ||
| Directory in the host operating system that should be mounted within the container. | ||
| """ | ||
| return posixpath.sep + re.sub("^([A-Za-z])+:", |
There was a problem hiding this comment.
This is a pretty complex expression. Not easy to follow for someone without context. Can you add a few examples to docstring of input and output of this method? It will help set context
samcli/local/docker/utils.py
Outdated
| import pathlib2 as pathlib | ||
|
|
||
|
|
||
| def posix_path(code_path): |
There was a problem hiding this comment.
nit: posix_path or to_posix_path?
| Parameters | ||
| ---------- | ||
| code_path : str | ||
| Directory in the host operating system that should be mounted within the container. |
There was a problem hiding this comment.
Added Returns and Examples.
| @@ -46,6 +46,32 @@ def test_init_must_store_all_values(self): | |||
| self.assertEquals(None, container.id) | |||
| self.assertEquals(self.mock_docker_client, container.docker_client) | |||
|
|
|||
| @patch("samcli.local.docker.utils.os") | |||
| def test_init_change_host_dir_unix_style(self, os_mock): | |||
| os_mock.name = "nt" | |||
There was a problem hiding this comment.
Can you have a test for the non-nt case?
- On supplying path for docker, change path to unix style. - This allows sam-cli to work with docker toolbox on windows.
sriram-mv
force-pushed
the
windows_style_playground
branch
from
07cce2d
to
1e6bda5
Compare
|
Addressed the comments. |
| -------- | ||
| >>> to_posix_path('/Users/UserName/sam-app') | ||
| /Users/UserName/sam-app | ||
| >>> to_posix_path('C:\\\\Users\\\\UserName\\\\AppData\\\\Local\\\\Temp\\\\mydir') |
There was a problem hiding this comment.
Had to do multiple escapes, because of quotes within docstrings, thats still a valid path and resolves.
In [332]: to_posix_path('C:\\Users\\UserName\\AppData\\Local\\Temp\\mydir')
Out[332]: '/c/Users/UserName/AppData/Local/Temp/mydir'
In [333]: to_posix_path('C:\\\\Users\\\\UserName\\\\AppData\\\\Local\\\\Temp\\\\mydir')
Out[333]: '/c/Users/UserName/AppData/Local/Temp/mydir'
| self.mock_docker_client.containers.create.assert_called_with(self.image, | ||
| command=self.cmd, | ||
| working_dir=self.working_dir, | ||
| volumes=translated_volumes, |
There was a problem hiding this comment.
Additional volumes with windows style paths, also resolve to unix style paths.
| @@ -104,6 +105,9 @@ def create(self): | |||
| if self._additional_volumes: | |||
| kwargs["volumes"].update(self._additional_volumes) | |||
|
|
|||
| # Make sure all mounts are of posix path style. | |||
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.