Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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 @@ -49,7 +49,7 @@

<artifactId>logstash</artifactId>
<packaging>hpi</packaging>
<version>1.4.0-SNAPSHOT</version>
<version>1.4.1-SNAPSHOT</version>
<name>Logstash</name>
<description>A Logstash agent to send Jenkins logs to a Logstash indexer.</description>
<url>https://wiki.jenkins-ci.org/display/JENKINS/Logstash+Plugin</url>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public DescriptorImpl getDescriptor() {

// Method to encapsulate calls for unit-testing
LogstashWriter getLogStashWriter(AbstractBuild<?, ?> build, OutputStream errorStream) {
return new LogstashWriter(build, errorStream, null);
return new LogstashWriter(build, errorStream, null, "{}");
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,13 @@ public class LogstashNotifier extends Notifier implements SimpleBuildStep {

public int maxLines;
public boolean failBuild;
public String customData;

@DataBoundConstructor
public LogstashNotifier(int maxLines, boolean failBuild) {
public LogstashNotifier(int maxLines, boolean failBuild, String customData) {

Choose a reason for hiding this comment

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

this will make customData a mandatory argument
this is a no-no as it will break existing usage
please use @DataBoundSetter instead

this.maxLines = maxLines;
this.failBuild = failBuild;
this.customData = customData;
}

@Override
Expand All @@ -85,7 +87,7 @@ private boolean perform(Run<?, ?> run, TaskListener listener) {

// Method to encapsulate calls for unit-testing
LogstashWriter getLogStashWriter(Run<?, ?> run, OutputStream errorStream, TaskListener listener) {
return new LogstashWriter(run, errorStream, listener);
return new LogstashWriter(run, errorStream, listener, customData);
}

public BuildStepMonitor getRequiredMonitorService() {
Expand Down
11 changes: 6 additions & 5 deletions src/main/java/jenkins/plugins/logstash/LogstashWriter.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public class LogstashWriter {
final LogstashIndexerDao dao;
private boolean connectionBroken;

public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener) {
public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener, String customData) {
this.errorStream = error != null ? error : System.err;
this.build = run;
this.listener = listener;
Expand All @@ -71,7 +71,7 @@ public LogstashWriter(Run<?, ?> run, OutputStream error, TaskListener listener)
this.buildData = null;
} else {
this.jenkinsUrl = getJenkinsUrl();
this.buildData = getBuildData();
this.buildData = getBuildData(customData);
}
}

Expand Down Expand Up @@ -133,11 +133,12 @@ LogstashIndexerDao getDao() throws InstantiationException {
return IndexerDaoFactory.getInstance(descriptor.type, descriptor.host, descriptor.port, descriptor.key, descriptor.username, descriptor.password);
}

BuildData getBuildData() {
BuildData getBuildData(String customData) {

if (build instanceof AbstractBuild) {
return new BuildData((AbstractBuild) build, new Date(), listener);
return new BuildData((AbstractBuild) build, new Date(), listener, customData);
} else {
return new BuildData(build, new Date(), listener);
return new BuildData(build, new Date(), listener, customData);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import static java.util.logging.Level.WARNING;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import net.sf.json.JSONException;
import net.sf.json.JSONObject;

import org.apache.commons.lang.StringUtils;
Expand Down Expand Up @@ -120,6 +121,7 @@ public TestData(Action action) {
protected String displayName;
protected String fullDisplayName;
protected String description;
protected JSONObject custom;
Copy link

@jakub-bochenski jakub-bochenski Dec 12, 2017

Choose a reason for hiding this comment

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

BuildData doesn't seem a good place to store this.
This data is not related to a build instead being taken from static configuration. We also need a way to add this that doesn't force a change in so many places (make it more SOLID)

protected String url;
protected String buildHost;
protected String buildLabel;
Expand All @@ -135,7 +137,7 @@ public TestData(Action action) {
protected TestData testResults = null;

// Freestyle project build
public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener) {
public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener, String customData) {
initData(build, currentTime);

Node node = build.getBuiltOn();
Expand Down Expand Up @@ -171,6 +173,14 @@ public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener liste
}
}
}

// Parse JSON string. If fails add error message and continue
try{
custom = JSONObject.fromObject(customData);
}catch (JSONException e){
custom = JSONObject.fromObject("{\"error\":\"custom data json parse failed\"}");
}

try {
buildVariables.putAll(build.getEnvironment(listener));
} catch (Exception e) {
Expand All @@ -183,7 +193,7 @@ public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener liste
}

// Pipeline project build
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener, String customData) {
initData(build, currentTime);

Executor executor = build.getExecutor();
Expand All @@ -198,6 +208,13 @@ public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
rootProjectDisplayName = displayName;
rootBuildNum = buildNum;

// Parse JSON string. If fails add error message and continue
try{
custom = JSONObject.fromObject(customData);
}catch (JSONException e){
custom = JSONObject.fromObject("{\"error\":\"custom data json parse failed\"}");
}

try {
// TODO: sensitive variables are not filtered, c.f. https://stackoverflow.com/questions/30916085
buildVariables = build.getEnvironment(listener);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
<f:entry title="${%Max lines}" field="maxLines">
<f:number default="1000" value="${instance.maxLines}" checkUrl="'descriptorByName/LogstashInstallation/checkInteger?value='+escape(this.value)" />
</f:entry>
<f:entry title="${%Custom Data}" field="customData">
<f:textbox default="{}" />
</f:entry>
<f:entry title="${%Can fail build}" field="failBuild">
<f:checkbox checked="${instance.failBuild}" />
</f:entry>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
<p>Custom json string to send to Logstash.<br/>

Choose a reason for hiding this comment

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

please change the code to parse and validate the json on input

Expected json string. If parse fail, error message is added in custom field</p>
</div>
22 changes: 11 additions & 11 deletions src/test/java/jenkins/plugins/logstash/LogstashNotifierTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ public class LogstashNotifierTest {
static class MockLogstashNotifier extends LogstashNotifier {
LogstashWriter writer;

MockLogstashNotifier(int maxLines, boolean failBuild, LogstashWriter writer) {
super(maxLines, failBuild);
MockLogstashNotifier(int maxLines, boolean failBuild, LogstashWriter writer, String customData) {
super(maxLines, failBuild, customData);
this.writer = writer;
}

Expand Down Expand Up @@ -95,7 +95,7 @@ public void before() throws Exception {
when(mockWriter.isConnectionBroken()).thenReturn(false);
Mockito.doNothing().when(mockWriter).writeBuildLog(anyInt());

notifier = new MockLogstashNotifier(3, false, mockWriter);
notifier = new MockLogstashNotifier(3, false, mockWriter, "{\"key\":\"value\"}");
}

@After
Expand Down Expand Up @@ -143,7 +143,7 @@ public void performBadWriterDoNotFailBuild() throws Exception {
// Initialize mocks
when(mockWriter.isConnectionBroken()).thenReturn(true);

notifier = new MockLogstashNotifier(3, false, mockWriter);
notifier = new MockLogstashNotifier(3, false, mockWriter, "{}");

// Unit under test
boolean result = notifier.perform(mockBuild, mockLauncher, mockListener);
Expand All @@ -163,7 +163,7 @@ public void performStepBadWriterDoNotFailBuild() throws Exception {
// Initialize mocks
when(mockWriter.isConnectionBroken()).thenReturn(true);

notifier = new MockLogstashNotifier(3, false, mockWriter);
notifier = new MockLogstashNotifier(3, false, mockWriter,"{}");

// Unit under test
notifier.perform(mockRun, null, mockLauncher, mockListener);
Expand All @@ -183,7 +183,7 @@ public void performBadWriterDoFailBuild() throws Exception {
// Initialize mocks
when(mockWriter.isConnectionBroken()).thenReturn(true);

notifier = new MockLogstashNotifier(3, true, mockWriter);
notifier = new MockLogstashNotifier(3, true, mockWriter, "{}");

// Unit under test
boolean result = notifier.perform(mockBuild, mockLauncher, mockListener);
Expand All @@ -203,7 +203,7 @@ public void performStepBadWriterDoFailBuild() throws Exception {
// Initialize mocks
when(mockWriter.isConnectionBroken()).thenReturn(true);

notifier = new MockLogstashNotifier(3, true, mockWriter);
notifier = new MockLogstashNotifier(3, true, mockWriter, "{}");

// Unit under test
notifier.perform(mockRun, null, mockLauncher, mockListener);
Expand Down Expand Up @@ -234,7 +234,7 @@ public Object answer(InvocationOnMock invocationOnMock) throws Throwable {
}
}).when(mockWriter).writeBuildLog(anyInt());

notifier = new MockLogstashNotifier(3, true, mockWriter);
notifier = new MockLogstashNotifier(3, true, mockWriter, "{}");
assertEquals("Errors were written", "", errorBuffer.toString());

// Unit under test
Expand All @@ -257,7 +257,7 @@ public void performAllLines() throws Exception {

Mockito.doNothing().when(mockWriter).writeBuildLog(anyInt());

notifier = new MockLogstashNotifier(-1, true, mockWriter);
notifier = new MockLogstashNotifier(-1, true, mockWriter, "{}");

// Unit under test
boolean result = notifier.perform(mockBuild, mockLauncher, mockListener);
Expand All @@ -279,7 +279,7 @@ public void performZeroLines() throws Exception {

Mockito.doNothing().when(mockWriter).writeBuildLog(anyInt());

notifier = new MockLogstashNotifier(0, true, mockWriter);
notifier = new MockLogstashNotifier(0, true, mockWriter, "{}");

// Unit under test
boolean result = notifier.perform(mockBuild, mockLauncher, mockListener);
Expand All @@ -296,7 +296,7 @@ public void performZeroLines() throws Exception {

@Test
public void getRequiredMonitorService() throws Exception {
Notifier notifier = new LogstashNotifier(1, false);
Notifier notifier = new LogstashNotifier(1, false, "{}");

// Unit under test
BuildStepMonitor synchronizationMonitor = notifier.getRequiredMonitorService();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ static LogstashWriter createLogstashWriter(final AbstractBuild<?, ?> testBuild,
final String url,
final LogstashIndexerDao indexer,
final BuildData data) {
return new LogstashWriter(testBuild, error, null) {
return new LogstashWriter(testBuild, error, null, "{}") {
@Override
LogstashIndexerDao getDao() throws InstantiationException {
if (indexer == null) {
Expand All @@ -48,12 +48,12 @@ LogstashIndexerDao getDao() throws InstantiationException {
}

@Override
BuildData getBuildData() {
BuildData getBuildData(String customData) {
assertNotNull("BuildData should never be requested for missing dao.", this.dao);

// For testing, providing null data means use the actual method
if (data == null) {
return super.getBuildData();
return super.getBuildData(customData);
} else {
return data;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class BuildDataTest {

static final String FULL_STRING = "{\"id\":\"TEST_JOB_123\",\"result\":\"SUCCESS\",\"fullProjectName\":\"parent/BuildDataTest\","
+ "\"projectName\":\"BuildDataTest\",\"displayName\":\"BuildData Test\",\"fullDisplayName\":\"BuildData Test #123456\","
+ "\"description\":\"Mock project for testing BuildData\",\"url\":\"http://localhost:8080/jenkins/jobs/PROJECT_NAME/123\","
+ "\"description\":\"Mock project for testing BuildData\", \"custom\": {}, \"url\":\"http://localhost:8080/jenkins/jobs/PROJECT_NAME/123\","
+ "\"buildHost\":\"master\",\"buildLabel\":\"master\",\"buildNum\":123456,\"buildDuration\":60,"
+ "\"rootProjectName\":\"RootBuildDataTest\",\"rootFullProjectName\":\"parent/RootBuildDataTest\","
+ "\"rootProjectDisplayName\":\"Root BuildData Test\",\"rootBuildNum\":456,\"buildVariables\":{},"
Expand Down Expand Up @@ -155,7 +155,7 @@ public void constructorSuccessBuiltOnNull() throws Exception {
when(mockBuild.getBuiltOn()).thenReturn(null);

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

// build.getDuration() is always 0 in Notifiers
Assert.assertEquals("Incorrect buildDuration", 60L, buildData.getBuildDuration());
Expand All @@ -176,7 +176,7 @@ public void constructorSuccessBuiltOnMaster() throws Exception {
when(mockNode.getLabelString()).thenReturn("");

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

// build.getDuration() is always 0 in Notifiers
Assert.assertEquals("Incorrect buildDuration", 60L, buildData.getBuildDuration());
Expand All @@ -197,7 +197,7 @@ public void constructorSuccessBuiltOnSlave() throws Exception {
when(mockNode.getLabelString()).thenReturn("Test Slave");

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

// build.getDuration() is always 0 in Notifiers
Assert.assertEquals("Incorrect buildDuration", 60L, buildData.getBuildDuration());
Expand All @@ -222,7 +222,7 @@ public void constructorSuccessTestFailures() throws Exception {
when(mockTestResultAction.getFailedTests()).thenReturn(Arrays.asList(mockTestResult));

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener,"{}");

Assert.assertEquals("Incorrect test results", 123, buildData.testResults.totalCount);
Assert.assertEquals("Incorrect test results", 0, buildData.testResults.skipCount);
Expand All @@ -240,7 +240,7 @@ public void constructorSuccessNoTests() throws Exception {
when(mockBuild.getAction(AbstractTestResultAction.class)).thenReturn(null);

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

Assert.assertEquals("Incorrect test results", null, buildData.testResults);

Expand Down Expand Up @@ -277,7 +277,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
when(mockBuild.getSensitiveBuildVariables()).thenReturn(new HashSet<>(Arrays.asList(sensitiveVarKey)));

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

// Verify results
Assert.assertEquals("Wrong number of environment variables", 2, buildData.getBuildVariables().size());
Expand Down Expand Up @@ -317,7 +317,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
when(mockBuild.getEnvironment(mockListener)).thenReturn(new EnvVars(varKey, buildVarVal));

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener,"{}");

// Verify results
Assert.assertEquals("Wrong number of environment variables", 1, buildData.getBuildVariables().size());
Expand All @@ -335,7 +335,7 @@ public void toJsonSuccess() throws Exception
when(mockBuild.getId()).thenReturn("TEST_JOB_123");
when(mockBuild.getUrl()).thenReturn("http://localhost:8080/jenkins/jobs/PROJECT_NAME/123");

BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener, "{}");

// Unit under test
JSONObject result = buildData.toJson();
Expand All @@ -353,7 +353,7 @@ public void fullName() throws Exception
when(mockBuild.getId()).thenReturn("TEST_JOB_123");
when(mockBuild.getUrl()).thenReturn("http://localhost:8080/jenkins/jobs/PROJECT_NAME/123");

BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener,"{}");

Assert.assertEquals(buildData.getFullProjectName(), "parent/BuildDataTest");

Expand All @@ -367,7 +367,7 @@ public void rootProjectFullName() throws Exception
when(mockBuild.getId()).thenReturn("TEST_JOB_123");
when(mockBuild.getUrl()).thenReturn("http://localhost:8080/jenkins/jobs/PROJECT_NAME/123");

BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener,"{}");

Assert.assertEquals(buildData.getRootFullProjectName(), "parent/RootBuildDataTest");

Expand Down