-
Notifications
You must be signed in to change notification settings - Fork 413
[JENKINS-74912] Fix Docker Windows non-C workspaces #332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like silently mounting the whole drive would violate the principle of least surprise.
The alternative would be to put the logic in WithContainerStep since that directly controls what is mounted. I'm making that change now. |
278713b
to
9311627
Compare
Same feedback applies. Seems surprising to mount the whole drive when the user requested only a subdirectory to be mounted. What if the drive contained a secret that the user did not want to expose? |
I updated the title to avoid confusion. The latest change only works around if the workspace is on a different drive. Users wouldn't interface with this logic. The plugin controls when the workspace is bind mounted to the container when launching it. Any manual user arguments (such as volumes) would still be passed in verbatim. I updated the title to reflect this - instead of changing all volumes the workaround only applies if the Jenkins workspace is on a different drive. Users can control the value the pass with -v so can (and will have to) workaround any non-C mounts themselves by mounting the whole drive explicitly. Permissions would be no different than a job that ran directly on the agent as the entire drive would already be mounted and available. If D:/foo is accessible from the agent, then D:/foo will be accessible by the container. If D:/foo is not accessible by the agent then D:/foo will not be accessible by the container provided the container runs as the same user. In other words mounting a separate volume in the container would look the same as not running inside the container. |
Sorry, I can't understand the above explanation. That might be because I don't know much about Windows Docker or this plugin. Might be best to wait for another reviewer or to adopt the plugin instead. |
Docker windows running windows does not support mounting non-C drive paths. It does however support mounting the entire drive. This change mounts the entire drive when we detect a non-C drive is being used for the workspace as a workaround.
Change includes a test that runs on Windows when Windows containers are enabled and a D drive exists (or gets skipped otherwise). Additionally tests now ensure docker is running in the corresponding OS mode using Assume.
Merge #326 first.
See JENKINS-74912
See Stack Overflow
See Github #41681
Testing done
Submitter checklist