-
Notifications
You must be signed in to change notification settings - Fork 414
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
JENKINS-60473 Containerize workspace paths when running Linux container on Windows host #197
base: master
Are you sure you want to change the base?
Conversation
Linux container on Windows host
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.
I agree that it would be a solution for the issue. looks like it is about causing some code duplication due to the third Docker client being introduced, but it looks OK. CC @Casz who contributed to the original code.
It would be great to add Javadoc to newly introduced APIs, and to also fix code style for opening curly brackets in some methods
else { | ||
dockerClient = new WindowsDockerClient(launcher, node, toolName); | ||
String os = dockerClient.inspect(new EnvVars(), step.image, ".Os"); | ||
if (os != null && os.equals("linux")) { |
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.
Should it be actually inverted? There is a lot of Unix operating systems being containerized, but Windows Client is for Windows only?
import java.util.logging.Level; | ||
import java.util.logging.Logger; | ||
|
||
public class WindowsLinuxDockerClient extends WindowsDockerClient { |
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.
Taking the code, it would be rather a Unix/POSIX container launcher. Not that important tho
@@ -378,4 +378,113 @@ public ContainerRecord getContainerRecord(@Nonnull EnvVars launchEnv, String con | |||
} | |||
return Arrays.asList(volumes.replace("\\", "/").split("\\n")); | |||
} | |||
|
|||
public String runCommand() { |
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.
add Javadoc for new API below?
I tested the branch on my Windows Host with execution the pipline example from Using Docker with Pipeline It results in an Error Message: More information i wrote also here: JENKINS-60473 |
@oleg-nenashev how can we make this merge happen ? |
Added to my list |
Any update here? Seems like this was ready to merge but abandoned, but the issue still remains |
This plugin is generally unmaintained and I do not recommend you use it. Instead directly run |
Sorry if necrobumping but any update on this? This feature would greatly streamline my build process. |
This attempts to address this reported issue:
https://issues.jenkins-ci.org/browse/JENKINS-60473
The main problem is that the plugin uses the host's absolute path for the workspace as a volume mount in the Docker container. This works fine when the OS matches between host and container (e.g., Linux/Linux or Windows/Windows). When running Linux containers on a Windows host, the Windows-style paths (with a drive letter) are not valid.
The code change in this fork detects that it's a mixed environment and converts workspace paths to Linux-compatible versions. This is running in production for my use-case, but it has not been robustly tested.