Fix relative paths (with dot) not working for job containers #1081
Conversation
| workDir = c.replaceWithMountPoint(conf.WorkingDirectory) | ||
| } | ||
|
|
||
| absPath, commandLine, err := getApplicationName(commandLine, workDir, os.Getenv("PATH")) |
There was a problem hiding this comment.
It's hard to follow what is happening with the commandLine here. Why is assigned above and then re-assigned here? Are they expected to be difference values?
There was a problem hiding this comment.
So the first thing we do is replace any instances of the env variable if there are any in the commandline.
Then on line 184 we reassign commandLine as it may have changed depending on if it needed to be quoted or not.
For instance, if someone passed foo bar baz, and foo bar.exe exists and baz was simply an argument, we need to quote the commandline to "foo bar" baz
There was a problem hiding this comment.
I'll comment the heck out of this, and re-push. It is a bit hard to follow
There was a problem hiding this comment.
I think it is outside the scope of this PR but the getApplicationName isn't clear as to what it does and that caused some confusion when reading. Comments help though it be better long term to refactor this a bit for clarity. Maybe its harder than it looks... just an observation
There was a problem hiding this comment.
It's a pretty close interpretation of the c++ logic for windows server containers, so this likely played a role in its complexity :P. I'll keep a note of this.
internal/jobcontainers/path_test.go
Outdated
| t.Fatal(err) | ||
| } | ||
|
|
||
| _, _, err = getApplicationName("./ping", cwd, os.Getenv("PATH")) |
There was a problem hiding this comment.
Why not test what the path and commandLine variables are here? Is not getting an error enough?
There was a problem hiding this comment.
This is mostly just testing that we found the application, so we resolved to C:\windows\system32\ping.exe, I can add an explicit check to make sure this is true however
internal/jobcontainers/path.go
Outdated
|
|
||
| // Asking for a relative path via "./", so trim the front. We handle relative paths such as "path/to/thing" and "path\\to\\thing", so just | ||
| // get rid of the prefix. | ||
| if len(commandLine) >= 3 && commandLine[0:2] == "./" || commandLine[0:2] == ".\\" { |
There was a problem hiding this comment.
wouldn't filepath.Clean work here?
There was a problem hiding this comment.
More than likely, but then I don't get to add all of this fun logic :). Joking of course, this slipped my mind
dcantah
force-pushed
the
jobcontainer-workdir
branch
2 times, most recently
from
30c307a to
981c7dd
Compare
Relative paths supplying a dot don't make it through our commandline parsing for job containers, this change fixes that. In addition to this, this change adds logic to actually honor the working directory specified for the container. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
jobcontainer-workdir
branch
from
981c7dd to
7a1ce51
Compare
|
@anmaxvl ptal again |
|
@jsturtevant Let me know if you have any other feedback |
|
/ltgm |
Related work items: microsoft#930, microsoft#962, microsoft#1004, microsoft#1008, microsoft#1039, microsoft#1045, microsoft#1046, microsoft#1047, microsoft#1052, microsoft#1053, microsoft#1054, microsoft#1057, microsoft#1058, microsoft#1060, microsoft#1061, microsoft#1063, microsoft#1064, microsoft#1068, microsoft#1069, microsoft#1070, microsoft#1071, microsoft#1074, microsoft#1078, microsoft#1079, microsoft#1081, microsoft#1082, microsoft#1083, microsoft#1084, microsoft#1088, microsoft#1090, microsoft#1091, microsoft#1093, microsoft#1094, microsoft#1096, microsoft#1098, microsoft#1099, microsoft#1102, microsoft#1103, microsoft#1105, microsoft#1106, microsoft#1108, microsoft#1109, microsoft#1115, microsoft#1116, microsoft#1122, microsoft#1123, microsoft#1126
Fix relative paths (with dot) not working for job containers
3 participants
Relative paths supplying a dot don't make it through our commandline
parsing for job containers, this change fixes that.
In addition to this, this change adds logic to actually honor the working
directory specified for the container.
Signed-off-by: Daniel Canter [email protected]