-
Notifications
You must be signed in to change notification settings - Fork 412
[FIXED] JENKINS-60473 #235
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
Conversation
@oleg-nenashev Can you please review? |
CC @jenkinsci/code-reviewers |
Can you revert the formatting changes, to make the code review easier? |
I will do it tomorrow EOD. |
Hi @jetersen, I have reverted the formatting changes, also moved all new methods to bottom of each file. Let me know if anything required from my end. Thanks |
@@ -61,6 +73,9 @@ public String run(@Nonnull EnvVars launchEnv, @Nonnull String image, @CheckForNu | |||
|
|||
@Override | |||
public List<String> listProcess(@Nonnull EnvVars launchEnv, @Nonnull String containerId) throws IOException, InterruptedException { | |||
if (isContainerUnix) { | |||
return listProcessUnixContainer(launchEnv, containerId); |
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.
Why not supper.listProcess(...) ? DockerClient.listProcess is already unix specific code. And include "-eo", "pid,comm" to remove the {} that busybox can put around the process names.
Just an attempt to improve #197
All credit goes to @mrsonicblue and @Bobby862
All I did optimized his code as he is away since 1 year (almost) and fixed java.io.NotSerializableException (his code was throwing this and even not properly running). Let me know what more to add or fix in this code.