[Rundeck] Support for shouldIncludeRundeckLogs

[szpak]

As shouldIncludeRundeckLogs requires shouldFailTheBuild to be set I also added a shortcut shouldWaitForRundeckJobAndIncludeRundeckLogs. It is convenient, but I don't know how it fits into job-dsl conventions.

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[daspilker]

How can I test this without a running instance of Rundeck?

[szpak]

In case it would be enough for you to verify that the same parameters are passed to a Rundeck instance you could compare XML created with manual marking that checkbox in Jenkins job configuration with the XML generated from Jenkins Job DSL with my change.
I have to admit I haven't test the Job DSL plugin version with my change and I end automatic testing on PublisherContextSpec which suggests that proper nodes are created in generated XML. How do you usually test the new features like that?

Regarding manual functional tests with Rundeck in my current build I have the following code and it works :).

configure { projectNode ->
    projectNode / 'publishers' / 'org.jenkinsci.plugins.rundeck.RundeckNotifier' << {
        'includeRundeckLogs'('true')
    }
}

[daspilker]

I always do some manual testing in Jenkins to ensure that there are no type in element names. I could not save the job configuration page because saving already seems to require a running instance of Rundeck.

[szpak]

Rundeck is a jar which can be run without installation. It is only required to create project and job and generate access token, but If you prefer I can send you a fragment of XML created with manual clicking in Jenkins.

[daspilker]

do not include shortcut methods, just set shouldWaitForRundeckJob when setting shouldIncludeRundeckLogs

[szpak]

I reworked the code after your comments.

[szpak]

By the way I also simplified a few tests in the neighborhood.

[burdandrei]

Great Feature! 👍

[daspilker]

do not fix unrelated tests, open a separate pull request for that

[daspilker]

This does not work in Jenkins, the checkbox is not set. Test this in Jenkins and fix the PR.

[szpak]

In the end there was no technical error in code, but Rundeck has inconsistently named variables and I had shouldIncludeRundeckLogs instead of includeRundeckLogs. I tested it manually with Jenkins 1.609.2.

Is there something else I should update in regards to recent documentation changes?

[daspilker]

Add GroovyDoc for the new DSL method. And so not refactor unrelated tests, keep the diff at a minimum.

[szpak]

Documentation moved from markdown file to GroovyDoc.

I reverted my changes to unrelated test in the previous commit... except one which was next to rundeck tests (which I missed). Currently only Rundeck related tests are modified.

[daspilker]

revert these changes, no need to use the safe navigation operator here

[szpak]

During the new feature development I came across a situation (after I renamed a node name) when assertion like that failed. The error message was:

java.lang.NullPointerException: Cannot invoke method value() on null object

From the error message I know that there wasn't node with given name, but I cannot say why - developer has to debug it or add some additional logging statement before.

After my change the error message became much more meaningful:

rundeckNode.includeRundeckLogsWrongName[0]?.value() == true
|           |                  |    |       |
|           []                 null null    false
org.jenkinsci.plugins.rundeck.RundeckNotifier[attributes={}; value=[jobId[attributes={}; value=jobId],
options[attributes={}; value=], nodeFilters[attributes={}; value=], tag[attributes={}; value=], 
shouldWaitForRundeckJob[attributes={}; value=true], shouldFailTheBuild[attributes={}; value=false],
includeRundeckLogs[attributes={}; value=true]]]

It is in some sense a boiler plate code, but unfortunately Spock currently doesn't support it. I personally prefer a need to add additional ? to get verbose error message in case of regression.

[kamilszymanski]

@daspilker any plans for merging this PR?
@szpak you need to rebase this PR - there are conflicts to be resolved

[szpak]

I rebased my PR - there are still conflicts in HOME.md.

I already removed all non rundeck related tests as requested by @daspilker.