Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.25</version>
<version>3.28</version>
<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
import org.jenkinsci.plugins.workflow.graph.FlowEndNode;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.job.console.NewNodeConsoleNote;
import org.jenkinsci.plugins.workflow.log.FileLogStorage;
import org.jenkinsci.plugins.workflow.log.LogStorage;
import org.jenkinsci.plugins.workflow.log.TaskListenerDecorator;
import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
Expand Down Expand Up @@ -1019,7 +1020,7 @@ private final class NodePrintListener implements GraphListener.Synchronous {
@Override public InputStream getLogInputStream() throws IOException {
// Inefficient but probably rarely used anyway.
ByteArrayOutputStream baos = new ByteArrayOutputStream();
writeLogTo(getLogText()::writeLogTo, baos);
writeLogTo(getLogText()::writeRawLogTo, baos);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, caused by my changes in #114. Good catch!

return new ByteArrayInputStream(baos.toByteArray());
}

Expand Down Expand Up @@ -1081,16 +1082,22 @@ private void writeLogTo(WriteMethod method, OutputStream os) throws IOException
@Deprecated
@Override public File getLogFile() {
LOGGER.log(Level.WARNING, "Avoid calling getLogFile on " + this, new UnsupportedOperationException());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the number of existing callers, should we only log the stack trace at FINE level?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well…we only know about those callers because it is at WARNING. And this would be important e.g. for getting sentry.io to show violations in Evergreen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this potentially going to generate a log of logspam though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated, which makes it feel very odd to me that we're deprecated its override in WorkflowJob

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this potentially going to generate a log of logspam though?

Potentially. Note that the log call code was already there—not touched by this PR.

I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated

Yup, on my to-do list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? While it may be satisfying to simple signal "stop using this API" by making it log a warning every time, for a user this looks like the Workflow Job plugin was broken by the update that added the logging - we've seen this before with "cosmetic" log warnings. Just not great optics or user experience.

Being able to point to deprecation of the extension point in core and an appropriate waiting period gives us more of a leg to stand on here.

Copy link
Member

@timja timja Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what do I call instead?
I'm making changes to the build-failure-analyzer plugin, and saw my logs being spammed here.
but there's no javadoc explaining what the replacement / another solution is?

It does look removeable as a whole potentially as all it does is get a file name off it but not sure the full implications

https://github.com/hmcts/build-failure-analyzer-plugin/blob/6176af08f11355b427a1db1a1c44c66dafe3de02/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/BuildLogFailureReader.java#L66

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timja Any log-related methods other than getLogFile. getLogText is the basic one. In your case I would suggest simply

-String currentFile = build.getLogFile().getName();
+String currentFile = "log";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will give it a go later on thanks 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that (at least on current Jenkins master branch) that Run.getLogFile is deprecated

jenkinsci/jenkins#3963

try {
File f = File.createTempFile("deprecated", ".log", getRootDir());
f.deleteOnExit();
try (OutputStream os = new FileOutputStream(f)) {
getLogText().writeRawLogTo(0, os);
File f = super.getLogFile();
if (LogStorage.of(asFlowExecutionOwner()) instanceof FileLogStorage) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more direct way to accomplish this, by any chance? It seems like it's going to carry a fair bit of logic along with that invocation, i.e. some extra risk -- and IIUC won't https://github.com/jenkinsci/workflow-api-plugin/blob/master/src/main/java/org/jenkinsci/plugins/workflow/log/LogStorage.java#L127 result in it returning a FileLogStorage even if there's no index file associated (i.e. non-rewrite builds)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this might be an escape hatch for us.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning a FileLogStorage even if there's no index file associated

Yup, and that is right. If there is no index file, log can still be read for the whole-log build. You will not get annotations about which lines came from particular steps, that is all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put it this way: that feels like an extremely surprising outcome for someone interacting with that API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not be interacting with this API at all—it is deprecated—but if you do, it behaves exactly as it did before, and continues to do for freestyle builds: gives you a File containing the complete raw text (i.e., including encoded ConsoleNotes) of the build log.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean specifically the LogStorage.of API - agree that in an ideal world nobody would be trying to work with the file directly (although I bet it's still happening anyway)

// FileLogStorage does write a file with the same name and the same format as before JEP-210, so accept it if it is there.
// This is the normal case if no additional plugin is installed.
} else {
// Some other storage, perhaps cloud-based. Cache under the traditional name, but load from the defined source on each call.
try {
f.deleteOnExit();
Copy link
Member

@svanoort svanoort Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, isn't f potentially the original log file, not a "deprecated" temp file that we're copying to/from?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means we'd... delete the build log on exit? Unless there's something I missed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be the FileLogStorage branch. I.e., what is run if you do not have the CloudWatch Logs plugin installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but in that case... isn't that the actual build log?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotta say it: we really, really, really should have a testcase covering these scenarios. Relying on code review alone to catch issues here is not a longterm path for success.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of FileLogStorage, yes that is the actual build log, as it would be for freestyle builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 So we're marking for Jenkins to... delete our build's actual logfile on exit. 🐛

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, except... due to magic the legacy logs magically have become a FileLogStorage due to the other weirdness of the LogStorage.of call. Which raises the question: what is this code addressing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless some plugin adds an alternative, FileLogStorage is in use. It does not matter if this build predates JEP-210.

try (OutputStream os = new FileOutputStream(f)) {
writeLogTo(getLogText()::writeRawLogTo, os);
}
} catch (IOException x) {
throw new RuntimeException(x);
}
return f;
} catch (IOException x) {
throw new RuntimeException(x);
}
return f;
}

static void alias() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import org.xml.sax.SAXException;

import javax.annotation.Nonnull;
import org.apache.commons.io.IOUtils;

public class WorkflowRunTest {

Expand Down Expand Up @@ -503,4 +504,13 @@ private void assertCulprits(WorkflowRun b, String... expectedIds) throws IOExcep
r.assertLogContains(message, r.buildAndAssertSuccess(p));
}

@Issue("JENKINS-54128")
@SuppressWarnings("deprecation")
@Test public void getLogFile() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("echo 'sample text'", true));
WorkflowRun b = r.buildAndAssertSuccess(p);
assertEquals(IOUtils.toString(b.getLogInputStream()), FileUtils.readFileToString(b.getLogFile()));
}

}