Skip to content
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.

Sign up for GitHub

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

Jump to bottom

properly get displayname of node #38

Merged
merged 2 commits into from
Dec 22, 2017
Merged

properly get displayname of node #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
371e27b
properly get displayname of node
mwinter69 Dec 11, 2017
67bc346
displayname of node in pipeline
mwinter69 Dec 16, 2017
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
19 changes: 14 additions & 5 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>1.599</version>
<version>1.608</version>
</parent>

<repositories>
Expand Down Expand Up @@ -65,8 +65,6 @@
<groupId>redis.clients</groupId>
<artifactId>jedis</artifactId>
<version>2.6.0</version>
<type>jar</type>
<scope>compile</scope>
</dependency>

<dependency>
Expand All @@ -88,8 +86,7 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.1.0</version>
<type>jar</type>
<version>2.10.0</version>
<scope>test</scope>
</dependency>

Expand Down Expand Up @@ -127,6 +124,18 @@
<artifactId>structs</artifactId>
<version>1.2</version>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-api-mockito2</artifactId>
<version>2.0.0-beta.5</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-module-junit4</artifactId>
<version>2.0.0-beta.5</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/jenkins/plugins/logstash/persistence/AbstractLogstashIndexerDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@
* @author Rusty Gerard
* @since 1.0.0
*/
abstract class AbstractLogstashIndexerDao implements LogstashIndexerDao {
public abstract class AbstractLogstashIndexerDao implements LogstashIndexerDao {
protected final String host;
protected final int port;
protected final String key;
protected final String username;
protected final String password;

AbstractLogstashIndexerDao(String host, int port, String key, String username, String password) {
public AbstractLogstashIndexerDao(String host, int port, String key, String username, String password) {
this.host = host;
this.port = port;
this.key = key;
Expand Down
10 changes: 6 additions & 4 deletions src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public TestData(Action action) {
public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener) {
initData(build, currentTime);

Node node = build.getBuiltOn();
Node node = build.getExecutor().getOwner().getNode();
Copy link

Choose a reason for hiding this comment

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

how about we try to use getBuiltOn() first and fall back to getExecutor().getOwner().getNode() if the first one is null? Just to be on the safe side

Copy link
Author

Choose a reason for hiding this comment

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

It must be null if you follow the logic.
In the method execute of class Run you will see that the BuildWrappers are initialized from method createBuildListener. But the builtOn is only set when later in the execute the RunExecution.run is called (which actually is AbstractBuild.AbstractBuildExecution.run)

Copy link

Choose a reason for hiding this comment

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

I get that. However this is relying on implementation details. With the current low test coverage I'd prefer to be as conservative as possible.

Copy link

Choose a reason for hiding this comment

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

Another way to look at it is this.
It seems pointless for anyone to call getBuiltOn() if getExecutor().getOwner().getNode() can give you the same result and do it earlier.

I see there is an @Exported getBuiltOnStr() method so remote API is the only sane use case.
Is getBuiltOn() purely a legacy method then?

Copy link
Author

Choose a reason for hiding this comment

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

It is not pointless.
getExecutor() will only return something when a build is running. getBuiltOn() will also return the node when the build is finished when the node with this name still exists. Internally only the name of the node is stored and when you call getBuiltOn() it searches for the node with the name.
Maybe its time to write some tests with JenkinsRule where a real job is executed on a node.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation.

Based on that I'd still prefer to keep the fallback logic in case somebody somewhere tries to pass a non-running build here.

Maybe its time to write some tests with JenkinsRule where a real job is executed on a node.

Yeah this project definitely needs a heavy makeover (including this class).

I started to port the integ test we do on travis with docker in #36 but there is always something more urgent.
I also need to get more familiar with the jenkins test harness, hopefully #40 will be an opportunity to learn by example

Copy link

Choose a reason for hiding this comment

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

Another thing: how does this compare to the pipeline logic in lines 190-194? Seems to to the same thing but using displayName?

Copy link
Author

Choose a reason for hiding this comment

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

This is wrong, it will return something like Executor-#0, but definitely not the host.
For pipelines using getExecutor().getOwner().getNode() will just get the host where the logstashSend is called, but the build could have run on many different hosts before.
Anyway I'm also looking into a more pipeline like logstash that accepts a block

logstash {
  node('label') {
    do something
    do anotherthing
  }
}

but I have to find a way to inject the host after the logstashwriter is initialized.

Copy link

Choose a reason for hiding this comment

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

will just get the host where the logstashSend is called, but the build could have run on many different hosts before.

I get that.
One final question: for now, do you think we should change the pipeline handling to use the getExecutor().getOwner().getNode() instead of the current executor display name?

Copy link
Author

Choose a reason for hiding this comment

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

I fixed also the pipeline handling to display the name of the node and not the executor. I also added some integration tests using JenkinsRule.

if (node == null) {
buildHost = "master";
buildLabel = "master";
Expand Down Expand Up @@ -186,11 +186,13 @@ public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener liste
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
initData(build, currentTime);

Executor executor = build.getExecutor();
if (executor == null) {
Node node = build.getExecutor().getOwner().getNode();
if (node == null) {
buildHost = "master";
buildLabel = "master";
} else {
buildHost = StringUtils.isBlank(executor.getDisplayName()) ? "master" : executor.getDisplayName();
buildHost = StringUtils.isBlank(node.getDisplayName()) ? "master" : node.getDisplayName();
buildLabel = StringUtils.isBlank(node.getLabelString()) ? "master" : node.getLabelString();
}

rootProjectName = projectName;
Expand Down
163 changes: 163 additions & 0 deletions src/test/java/jenkins/plugins/logstash/LogstashIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package jenkins.plugins.logstash;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertThat;
import static org.powermock.api.mockito.PowerMockito.when;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.jvnet.hudson.test.JenkinsRule;
import org.powermock.api.mockito.PowerMockito;
import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.Result;
import hudson.model.Slave;
import hudson.model.queue.QueueTaskFuture;
import jenkins.plugins.logstash.persistence.AbstractLogstashIndexerDao;
import jenkins.plugins.logstash.persistence.IndexerDaoFactory;
import jenkins.plugins.logstash.persistence.LogstashIndexerDao.IndexerType;
import net.sf.json.JSONArray;
import net.sf.json.JSONObject;

@RunWith(PowerMockRunner.class)
@PowerMockIgnore({"javax.crypto.*"})
@PrepareForTest(IndexerDaoFactory.class)
public class LogstashIntegrationTest
{
@Rule
public JenkinsRule jenkins = new JenkinsRule();

private Slave slave;

private FreeStyleProject project;

private MemoryDao memoryDao;

@Before
public void setup() throws Exception
{
PowerMockito.mockStatic(IndexerDaoFactory.class);
LogstashInstallation.Descriptor descriptor = LogstashInstallation.getLogstashDescriptor();
descriptor.type = IndexerType.SYSLOG;
descriptor.host = "localhost";
descriptor.port = 1;
descriptor.username = "username";
descriptor.password = "password";
descriptor.key = "key";

memoryDao = new MemoryDao();
when(IndexerDaoFactory.getInstance(IndexerType.SYSLOG, descriptor.host, descriptor.port, descriptor.key,
descriptor.username, descriptor.password)).thenReturn(memoryDao);
slave = jenkins.createSlave();
slave.setLabelString("myLabel");
project = jenkins.createFreeStyleProject();

}

@Test
public void test_buildWrapperOnMaster() throws Exception
{
project.getBuildWrappersList().add(new LogstashBuildWrapper());
QueueTaskFuture<FreeStyleBuild> f = project.scheduleBuild2(0);
FreeStyleBuild build = f.get();
assertThat(build.getResult(), equalTo(Result.SUCCESS));
List<JSONObject> dataLines = memoryDao.getOutput();
assertThat(dataLines.size(), is(3));
JSONObject firstLine = dataLines.get(0);
JSONObject lastLine = dataLines.get(dataLines.size()-1);
JSONObject data = firstLine.getJSONObject("data");
assertThat(data.getString("buildHost"),equalTo("Jenkins"));
assertThat(data.getString("buildLabel"),equalTo("master"));
assertThat(lastLine.getJSONArray("message").get(0).toString(),equalTo("Finished: SUCCESS"));
}

@Test
public void test_buildWrapperOnSlave() throws Exception
{
project.getBuildWrappersList().add(new LogstashBuildWrapper());
project.setAssignedNode(slave);

QueueTaskFuture<FreeStyleBuild> f = project.scheduleBuild2(0);
FreeStyleBuild build = f.get();
assertThat(build.getResult(), equalTo(Result.SUCCESS));
List<JSONObject> dataLines = memoryDao.getOutput();
assertThat(dataLines.size(), is(3));
JSONObject firstLine = dataLines.get(0);
JSONObject lastLine = dataLines.get(dataLines.size()-1);
JSONObject data = firstLine.getJSONObject("data");
assertThat(data.getString("buildHost"),equalTo(slave.getDisplayName()));
assertThat(data.getString("buildLabel"),equalTo(slave.getLabelString()));
assertThat(lastLine.getJSONArray("message").get(0).toString(),equalTo("Finished: SUCCESS"));
}

@Test
public void test_buildNotifierOnMaster() throws Exception
{
project.getPublishersList().add(new LogstashNotifier(10, false));
QueueTaskFuture<FreeStyleBuild> f = project.scheduleBuild2(0);
FreeStyleBuild build = f.get();
assertThat(build.getResult(), equalTo(Result.SUCCESS));
List<JSONObject> dataLines = memoryDao.getOutput();
assertThat(dataLines.size(), is(1));
JSONObject firstLine = dataLines.get(0);
JSONObject data = firstLine.getJSONObject("data");
assertThat(data.getString("buildHost"),equalTo("Jenkins"));
assertThat(data.getString("buildLabel"),equalTo("master"));
}

@Test
public void test_buildNotifierOnSlave() throws Exception
{
project.getPublishersList().add(new LogstashNotifier(10, false));
project.setAssignedNode(slave);
QueueTaskFuture<FreeStyleBuild> f = project.scheduleBuild2(0);
FreeStyleBuild build = f.get();
assertThat(build.getResult(), equalTo(Result.SUCCESS));
List<JSONObject> dataLines = memoryDao.getOutput();
assertThat(dataLines.size(), is(1));
JSONObject firstLine = dataLines.get(0);
JSONObject data = firstLine.getJSONObject("data");
assertThat(data.getString("buildHost"),equalTo(slave.getDisplayName()));
assertThat(data.getString("buildLabel"),equalTo(slave.getLabelString()));
}

private static class MemoryDao extends AbstractLogstashIndexerDao
{
List<JSONObject> output = new ArrayList<>();

public MemoryDao()
{
super("localhost", 1, "key", "username", "password");
}

@Override
public IndexerType getIndexerType()
{
// TODO Auto-generated method stub
return null;
}

@Override
public void push(String data) throws IOException
{
JSONObject json = JSONObject.fromObject(data);
output.add(json);
}

public List<JSONObject> getOutput()
{
return output;
}
}
}
8 changes: 5 additions & 3 deletions src/test/java/jenkins/plugins/logstash/LogstashNotifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintStream;
import java.util.Arrays;

import org.junit.After;
import org.junit.Before;
Expand All @@ -26,7 +25,7 @@
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.runners.MockitoJUnitRunner;
import org.mockito.junit.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;

@SuppressWarnings("rawtypes")
Expand Down Expand Up @@ -76,18 +75,21 @@ public Result getResult() {
@Mock LogstashWriter mockWriter;
@Mock Launcher mockLauncher;
@Mock BuildListener mockListener;
@Mock AbstractProject mockProject;

ByteArrayOutputStream errorBuffer;
PrintStream errorStream;
LogstashNotifier notifier;
MockRun mockRun;


@Before
public void before() throws Exception {
errorBuffer = new ByteArrayOutputStream();
errorStream = new PrintStream(errorBuffer, true);

mockRun = new MockRun(mock(AbstractProject.class));
when(mockProject.assignBuildNumber()).thenReturn(1);
mockRun = new MockRun(mockProject);

when(mockListener.getLogger()).thenReturn(errorStream);

Expand Down
16 changes: 11 additions & 5 deletions src/test/java/jenkins/plugins/logstash/LogstashWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import hudson.EnvVars;
import hudson.model.AbstractBuild;
import hudson.model.Computer;
import hudson.model.Executor;
import hudson.model.Project;
import hudson.model.Result;
import hudson.model.TaskListener;
Expand All @@ -15,7 +17,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.*;
import org.mockito.runners.MockitoJUnitRunner;
import org.mockito.junit.MockitoJUnitRunner;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -75,6 +77,9 @@ String getJenkinsUrl() {

@Mock BuildData mockBuildData;
@Mock TaskListener mockListener;
@Mock Computer mockComputer;
@Mock Executor mockExecutor;


@Captor ArgumentCaptor<List<String>> logLinesCaptor;

Expand All @@ -85,18 +90,19 @@ public void before() throws Exception {
when(mockBuild.getDisplayName()).thenReturn("LogstashNotifierTest");
when(mockBuild.getProject()).thenReturn(mockProject);
when(mockBuild.getParent()).thenReturn(mockProject);
when(mockBuild.getBuiltOn()).thenReturn(null);
when(mockBuild.getNumber()).thenReturn(123456);
when(mockBuild.getTimestamp()).thenReturn(new GregorianCalendar());
when(mockBuild.getRootBuild()).thenReturn(mockBuild);
when(mockBuild.getBuildVariables()).thenReturn(Collections.emptyMap());
when(mockBuild.getSensitiveBuildVariables()).thenReturn(Collections.emptySet());
when(mockBuild.getEnvironments()).thenReturn(null);
when(mockBuild.getAction(AbstractTestResultAction.class)).thenReturn(mockTestResultAction);
when(mockBuild.getLog(0)).thenReturn(Arrays.asList());
when(mockBuild.getLog(3)).thenReturn(Arrays.asList("line 1", "line 2", "line 3", "Log truncated..."));
when(mockBuild.getLog(Integer.MAX_VALUE)).thenReturn(Arrays.asList("line 1", "line 2", "line 3", "line 4"));
when(mockBuild.getEnvironment(null)).thenReturn(new EnvVars());
when(mockBuild.getExecutor()).thenReturn(mockExecutor);
when(mockExecutor.getOwner()).thenReturn(mockComputer);
when(mockComputer.getNode()).thenReturn(null);


when(mockTestResultAction.getTotalCount()).thenReturn(0);
when(mockTestResultAction.getSkipCount()).thenReturn(0);
Expand Down Expand Up @@ -142,7 +148,7 @@ public void constructorSuccess() throws Exception {
verify(mockBuild).getDescription();
verify(mockBuild).getUrl();
verify(mockBuild).getAction(AbstractTestResultAction.class);
verify(mockBuild).getBuiltOn();
verify(mockBuild).getExecutor();
verify(mockBuild, times(2)).getNumber();
verify(mockBuild).getTimestamp();
verify(mockBuild, times(4)).getRootBuild();
Expand Down
Loading