properly get displayname of node

[mwinter69]

use the executor of the build to get the node.
builtOn is set too late for us to get the node and we end up using
master
This is a proper implementation that was tried to be fixed with PR #19

[jakub-bochenski]

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

[mwinter69]

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)

[jakub-bochenski]

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.

[jakub-bochenski]

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?

[mwinter69]

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.

[jakub-bochenski]

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

[jakub-bochenski]

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

[mwinter69]

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.

[jakub-bochenski]

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?

[mwinter69]

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.

[jakub-bochenski]

Will try to merge this soon. just need to find time to read through the latest commit