2050 ensure windows volume paths are compatible with engine #2122
Conversation
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
2 times, most recently
from
be6e0e3 to
d3d0f2c
Compare
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
from
82deb05 to
62bb4a8
Compare
mnowster
changed the title
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
from
a9b656b to
00a2b97
Compare
mnowster
changed the title
mnowster
changed the title
compose/service.py
Outdated
There was a problem hiding this comment.
Reading ensure_engine_compatibility's docstring, I sort of expected this bit of logic to happen over there. Would it be simple enough to move it there?
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
4 times, most recently
from
b2115d3 to
df7d860
Compare
When a relative path is expanded and we're on a windows platform, it expands to include the drive, eg C:\ , which was causing a ConfigError as we split on ":" in parse_volume_spec and that was giving too many parts. Use os.path.splitdrive instead of manually calculating the drive. This should help us deal with windows drives as part of the volume path better than us doing it manually. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
2 times, most recently
from
1e7e77d to
6795ece
Compare
compose/service.py
Outdated
There was a problem hiding this comment.
Minor naming nit: this function name seems a lot more general then what it's doing. "ensure" to me means "validate", and "engine compatibility" is a much larger issue than just paths.
How about normalize_host_path() (or something along those lines) ?
There was a problem hiding this comment.
This was a tough one to name, I was nearly going to invent a word 🔮 but I chatted with @aanand about it as well. I originally was going to use transform_for_engine or something similar so that it is very explicit that we are changing the way these paths look for the specific need of the engine.
normalize_host_path doesn't capture the specific need of we are doing this for the engine. I understand the point that there are larger issues than paths, could be ensure_paths_are_engine_compatible is clearer?
There was a problem hiding this comment.
Does it matter why we're doing it? I usually keep naming to "what" it's doing, and leave the "why" for the docstring.
To me this is a "normalize" operation not "ensure". "ensure" is validation/assertions, this is "do something in some cases, but ignore it in others".
normalize_paths_for_engine() would work for me (even though "engine" might not always be relevant, it's more that we're normalizing them for posix, so my first choice would probably be normalize_paths_to_posix() or something like that).
There was a problem hiding this comment.
That naming, was me trying to describe "what" we're doing and part of that what included the "why". When naming things, especially tricky things like this, I try to think about the person who has to come after me and fix it or modify the code, so I lean towards verbosity and whys as that will aid them to understand why the code is the way it is and pick it up quicker than it took me to figure it out the first time.
Ultimately we're agreeing with each other but with different words :)
I'm happier with normalize_paths_for_engine for now, as this is a workaround a hack within the engine itself. Then when that is no longer the case, we can rename it again. Cool.
An expanded windows path of c:\shiny\thing:\shiny:rw needs to be changed to be linux style path, including the drive, like /c/shiny/thing /shiny to be mounted successfully by the engine. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
I'd written relative, when I meant absolute. D'oh. Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
mnowster
force-pushed
the
2050-relative-volume-paths-windows
branch
from
6795ece to
af8032a
Compare
This needs resolving outside of this PR, as it is a much bigger piece of work. docker#2128 Signed-off-by: Mazz Mosley <mazz@houseofmnowster.com>
|
LGTM |
…dows 2050 ensure windows volume paths are compatible with engine
4 participants
This fixes absolute windows paths having drives which caused the split on
:logic to break. It also ensures the path is compatible with engine, which requires a linux path, including the drive.Fixes #2050