Improve volumespec parsing on windows platforms #3922
Conversation
shin-
added
area/config
area/volumes
group/windows-client
and removed
group/windows
status/0-triage
labels
| return ('', '') | ||
| if path[0] in ['.', '\\', '/', '~']: | ||
| return ('', path) | ||
| return ntpath.splitdrive(path) |
There was a problem hiding this comment.
As far as I can tell, we expand volume host paths in compose.config.config.process_service() before we get to VolumeSpec.parse(). So paths here shouldn't ever start with . or ~. Or is VolumeSpec.parse() used in other contexts where paths might be relative?
There was a problem hiding this comment.
Yes, that shouldn't be an issue in that scenario. I think it's still nice to have a splitdrive function that can handle those corner cases and be used elsewhere, however, but if you feel strongly about it I can remove those from the list.
There was a problem hiding this comment.
I've moved splitdrive to compose.utils so we can use it in both VolumeSpec.parse and compose.config.split_path_mapping, with the latter occuring before the path expansion.
| if external_path: | ||
| external_path = _normalize(external_path) | ||
|
|
||
| return external_path, _normalize(internal_path) |
There was a problem hiding this comment.
Is normalize_paths_for_engine() still a useful function? Seems like we could just move _normalize() up to the top level (maybe renaming it to normalize_path_for_engine(), singular) and invoke it directly from VolumeSpec._parse_win32().
There was a problem hiding this comment.
I think you're right, we could do without it.
| os.path.normpath(external), os.path.normpath(parts[0]) | ||
| ) | ||
| if len(parts) > 1: | ||
| mode = parts[1] |
There was a problem hiding this comment.
Looks like there's some repeated logic here where we call splitdrive, call split(':') on the tail, then prepend the drive onto the first part. Could it be factored out into a helper function?
shin-
force-pushed
the
3897-volumespec_parse_win32
branch
2 times, most recently
from
a0b426c
to
6292024
Compare
| @@ -944,11 +944,7 @@ def split_path_mapping(volume_path): | |||
| """ | |||
| # splitdrive is very naive, so handle special cases where we can be sure | |||
| # the first character is not a drive. | |||
Signed-off-by: Joffrey F <joffrey@docker.com>
shin-
force-pushed
the
3897-volumespec_parse_win32
branch
from
6292024
to
7911659
Compare
Fixes #3897