-
Notifications
You must be signed in to change notification settings - Fork 164
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-54346] && [JENKINS-55505] Fix stream handling in tee (followup) #77
Conversation
Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
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.
Scarcely remember the considerations at this point, but I think this was basically right.
which is also guaranteeing backward compatibility when resuming after plug-in upgrade. | ||
*/ | ||
transferredToRemote = Channel.current() != null; | ||
oos.defaultWriteObject(); |
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.
In the original PR I suggested that this be rewritten to use writeReplace
for legibility and maintainability, but I do not consider it a blocker.
src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/fs/TeeStep.java
Outdated
Show resolved
Hide resolved
@Test | ||
@Issue({"JENKINS-54346", "JENKINS-55505"}) | ||
public void closed() throws Exception { | ||
rr.then(r -> { |
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.
Still no test coverage of behavior across restarts that I can tell.
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.
See my last commit.
Is that what you expect?
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.
Something like that, yes. I would have to immerse myself in this patch for a while to really remember what the tricky cases were.
Signed-off-by: Philipp Homann <homann.philipp@googlemail.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
This is just a followup of #62
Most credits of changes going out to @tolnaisz
Just fixed the test error mentioned in #62