Final line of shell output without newline lost when using LineTransformationOutputStream-based decorators #112
Conversation
| @@ -132,7 +132,7 @@ | |||
| <dependency> | |||
| <groupId>org.jenkins-ci.plugins</groupId> | |||
| <artifactId>credentials-binding</artifactId> | |||
| <version>1.16</version> | |||
| <version>1.17</version> | |||
There was a problem hiding this comment.
To pick up jenkinsci/credentials-binding-plugin#51. This seems to be the reason that, in watch mode, the test was previously failing (output was getting lost) even in situations that did not involve a missing final newline.
It is a bit scary, however, and produces poorly formatted log text like: missing final newline node=remote watching=true[Pipeline] } which would confuse, for example, pipeline-cloudwatch-logs.
jglick
changed the title
jglick
changed the title
… compatible with this plugin.
…oids the need for a workaround.
src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java
Show resolved
Hide resolved
| @@ -599,6 +652,7 @@ private void handleExit(int exitCode, OutputSupplier output) throws IOException, | |||
| getContext().onFailure(new AbortException("script returned exit code " + exitCode)); | |||
| } | |||
| } | |||
| listener().getLogger().close(); | |||
There was a problem hiding this comment.
So this call will always be on NewlineSafeTaskListener, right? Is this why we don't want to call super.close() in NewlineSafeTaskListener.$NewlineSafeTaskListener.close()? Ditto for the call below in exited. Also, if we are calling close here now, why didn't we call flush here earlier? Should we have been doing that?
There was a problem hiding this comment.
So this call will always be on
NewlineSafeTaskListener, right?
Right.
Is this why we don't want to call
super.close()inNewlineSafeTaskListener.$NewlineSafeTaskListener.close()?
Yes.
if we are calling
closehere now, why didn't we callflushhere earlier?
There is no particular need to flush until the end of the step (and close flushes).
There was a problem hiding this comment.
There is no particular need to flush until the end of the step (and close flushes).
My question was unclear, but my confusion is about why we weren't calling flush before this PR, since it looks like this is the end of the step.
There was a problem hiding this comment.
Oh, I see! We were calling flush before, in watch mode, in exited. In non-watch mode, it did not really matter since the log messages were coming from the master, and DefaultStepContext.getListener always closes the per-step listener when the step finishes.
There was a problem hiding this comment.
Seems fine. I am struggling to keep track of when to flush or not, what overrides of close also close their delegates, and what is getting remoted or not in Pipeline logging in general, so my approval is mostly just based on passing tests and no Java-level issues that I see.
| @@ -599,6 +652,7 @@ private void handleExit(int exitCode, OutputSupplier output) throws IOException, | |||
| getContext().onFailure(new AbortException("script returned exit code " + exitCode)); | |||
| } | |||
| } | |||
| listener().getLogger().close(); | |||
There was a problem hiding this comment.
There is no particular need to flush until the end of the step (and close flushes).
My question was unclear, but my confusion is about why we weren't calling flush before this PR, since it looks like this is the end of the step.
So am I! |
| @@ -65,6 +65,7 @@ | |||
| <revision>2.33</revision> | |||
| <changelist>-SNAPSHOT</changelist> | |||
| <jenkins.version>2.176.1</jenkins.version> | |||
| <jenkins-test-harness.version>2.54</jenkins-test-harness.version> <!-- TODO pending plugin-pom update --> | |||
…that fix is only available on 2.176.x+
Fixing the test from #103, strengthening it in the process. Taking on some urgency since after @Vlatombe’s jenkinsci/kubernetes-plugin#552, this problem would affect any
shstep in a Kubernetes agent which prints output without a trailing newline (no longer limited to that minority of builds which deliberately use secrets).kubernetestest workarounds can be removed: Demonstrating that an upstream fix avoids the need for a workaround kubernetes-plugin#554pipeline-cloudwatch-logs: Test of output lines missing final newline pipeline-cloudwatch-logs-plugin#58