-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
filter exit code from command output #242
Conversation
d6d8703
to
a97190b
Compare
Thanks for the pull request. I think you are right, it is probably a good idea in general to not pass the "EXITCODE" stuff to the client. Could you please add more information on "the docker client jenkins plugin [...] failing with kubernetes", i.e. an example pipeline that shows it and the error in the build log? |
I'm planning a change on this PR. I hope if it only runs the matching part of the code at the point where the plugin reads out the exitcode it won't need to match on all write. the docker client part:
The problem here is that the docker plugin expects the ProcStarter to only return the output of the command. I haven't looked at test code, but I'll add the test (I hope I can :D). One question: |
Cool. I added "[WIP]" to the title of this PR, please remove once it is ready to review again. |
I've been testing out the ssh-agent bug and it looks like I found a solution:
|
while this might get rid of the error, I guess it has to be considered a change of functionality to just throw away stderr... |
there is no stderr anyway. at least not from the command.
The output of the command is piped to stdout anyway, there is something else that sneaks something funny on the output and stderr is not collected separately:
IMHO it shouldn't be collected anyway. It can cause problems if it is pushed on stdout (which is happening now) and if another plugin expects it the current solution can't give it anything. |
19f408b
to
158774b
Compare
this should work now IMHO. |
this latest change should also fix the ssh-agent problem. |
@marvinthepa or maybe @carlossg ? |
7943f8c
to
fdbd931
Compare
Tested locally a few dozen times with a particular smoke test 👍 |
@carlossg |
sorry for the delay, I am trying to figure out if there is a better way to fetch the exit code because this is adding a lot of complexity to the code |
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.
Can you add a summary on how the stream flow works across all classes?
Thanks!
|
||
//check if the cmd is sourced from Jenkins, rather than another plugin; if so, skip cmdEnvs as we are getting other environment variables | ||
for (String cmd : cmdEnvs) { | ||
if (cmd.startsWith(JENKINS_HOME)) { |
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.
this is no longer needed ?
it's being reimplemented in
https://github.com/jenkinsci/kubernetes-plugin/pull/245/files#diff-8a1ab34452b01252e89ffeee0b2eda45R201
@@ -431,4 +437,37 @@ public int getExitCode() { | |||
return i; | |||
} | |||
} | |||
|
|||
static class FilterOutExitCodeOutputStream extends OutputStream { |
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.
can we move it to a new class org.csanchez.jenkins.plugins.kubernetes.pipeline.exec. FilterOutExitCodeOutputStream
with
@Restricted(NoExternalUse.class)
@@ -0,0 +1,69 @@ | |||
package org.csanchez.jenkins.plugins.kubernetes.pipeline; |
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.
can we put it under org.csanchez.jenkins.plugins.kubernetes.pipeline.exec
|
||
import org.apache.commons.lang.ArrayUtils; | ||
|
||
public class ContainerExecCutExitCodeUtil { |
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.
@Restricted(NoExternalUse.class)
merge with jenkinsci/kubernetes-plugin master and review modifications
I've made a graph about the output stream flow |
@carlossg |
@balihb for how much I'd love to have this merged, there is a better way than filtering streams this way. This fabric8io/kubernetes-client#1045 provides the base support for keeping the signalling stream separate. Would you mind giving it a look? |
Yes, this whole section needs to be rewritten with the improvements in fabric8io/kubernetes-client#1045 JENKINS-50392 |
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.
To be rewritten with the improvements in fabric8io/kubernetes-client#1045
Thanks for the effort though!
I think #300 will sort this out |
Ok, I won't say this is the nicest solution for this, but I have no better idea to prevent things like the docker client jenkins plugin from failing with kubernetes.