Skip to content

Conversation

@dcantah
Copy link
Contributor

@dcantah dcantah commented Aug 27, 2021

Instead of taking the working directory as is, change to joining the working directory
requested with where the sandbox volume is located. It's expected that the default behavior
would be to treat all paths as relative to the volume as this would be equivalent to a
normal Windows Server Containers behavior.

For example:
A working directory of C:\ would become C:\C\12345678
A working directory of C:\work\dir would become C:\C\12345678\work\dir

Signed-off-by: Daniel Canter [email protected]

@dcantah dcantah requested a review from a team as a code owner August 27, 2021 20:30
@anmaxvl anmaxvl self-assigned this Sep 9, 2021
@dcantah
Copy link
Contributor Author

dcantah commented Sep 13, 2021

Need to rebase, doing now

Instead of taking the working directory as is, change to joining the working directory
requested with where the sandbox volume is located. It's expected that the default behavior
would be to treat all paths as relative to the volume as this would be equivalent to a
normal Windows Server Containers behavior.

For example:
A working directory of C:\ would become C:\C\12345678\
A working directory of C:\work\dir would become C:\C\12345678\work\dir

Signed-off-by: Daniel Canter <[email protected]>
@dcantah dcantah force-pushed the jobcontainer-fixworkdir branch from 948bd28 to 3e7e5ef Compare September 13, 2021 19:50
@dcantah
Copy link
Contributor Author

dcantah commented Sep 13, 2021

Actually let me add a test for this also.

This change adds a couple tests to make sure that the working directory functions as
expected. Also some very small adjustments on the dockerfiles for the other tests
(which really didn't need to be changed, but makes it more explicit).

Signed-off-by: Daniel Canter <[email protected]>
@dcantah
Copy link
Contributor Author

dcantah commented Sep 13, 2021

@anmaxvl Need a re-review for the tests, sorry 🥲

@dcantah
Copy link
Contributor Author

dcantah commented Sep 13, 2021

@katiewasnothere @ambarve If either of you have time to look at this, let me know and I can assign. Need another set of eyes.

Copy link

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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.

3 participants