[FIXED JENKINS-27182]: Allow to test Job DSL scripts (dry run)

[arcivanov]

In simulation mode run the DSL scripts, print them out to console log but don't actually do anything

This mode allows to test pull requests for compilation errors and allow to verify output manually prior to merging the PR containing DSL

[jenkinsadmin]

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

[AnalogJ]

This is something we're looking for.

[lanwen]

+1 for this feature

[daspilker]

As I commented on the JIRA ticket, I still think that having unit test style tests is the better option. They can be run locally by each developer, even from the IDE, in a few seconds. The simulation mode would require to commit the changes and then wait for the CI, fix any problems, commit again and so on. That does not sound like a convenient development cycle.
Logging kilobytes of raw XML output to the console log when simulating 100s of jobs does not seem to be convenient for debugging to me. If you want to see the XML, you can do that in unit tests as well. You could even attach a debugger to see what goes wrong.
I agree that the setup is a little bit more complex, but when your Job DSL scripts get more complex, you should invest in unit tests.

[arcivanov]

@daspilker I've seen your comment but I think we're talking about different things.

Firstly, unit tests are impossible to write for anyone but developers and DSLs exist for people who can't write code. Even for developers testing the DSL is extremely expensive.
Secondly, no matter how good your unit tests are, if you screw up Credential ID in your DSL you will get a silent null in your XML and you're basically screwed.
Thirdly, staging guards deployment to Jenkins, DSL writer cannot be trusted and the tests should run on the Jenkins server where things are being deployed.

The scenario I'm talking about is as follows:

1. Jenkins is administered by people other than those writing DSLs. Jenkins admins maintain security and manage credentials
2. DSL writers are not coders, they can't write unit tests and don't know Groovy
3. There are only two manually created jobs in the entire server:
   • Job Generator from Pull Request in Simulation mode
   • Job Generator
4. DSL writer edits DSL, submits pull request. Whether unit tests run on that DSL locally is irrelevant: Jenkins configuration is fluid and unless you have Jenkins configuration locally synchronized at all times there is no guarantee that the jobs generated will actually work. The PR is built in simulation mode and SOME information is presented to the submitting DSL editor allowing her inspect the changes that will happen due to PR.
5. Someone other than DSL writer merges the PR after review (and there has to be something to review other than DSL, like staging deployment results), Job Generator (re-)generates some or all jobs on the Jenkins.

I hate having to read XML but having simulation mode in its current form is the cheapest implementation way to currently inspect what the jobs will ACTUALLY look on the Jenkins where you're deploying them.

Ideally there would be some job generation failures that would prevent job from being generated when referenced Jenkins resources cannot be found, or we would stage changes by generating disabled jobs with some prefix (e.g. PR<#>-) and some button would allow to drop such staged job, etc etc.

I'm very open to suggestions on how this can be implemented given Jenkins limitations.

[daspilker]

So, I guess there are still enough people left, who do not do this "DevOps" thing... 😄

Let's have a look at your changes. You need to add the simulation mode to all JobManagement methods which modify state, e.g. createOrUpdateConfigFile, createOrUpdateUserContent and queueJob. And the data in javaposse.jobdsl.plugin.DescriptorImpl should not be modified when running a simulation. Then add tests to JenkinsJobManagementSpec and ExecuteDslScriptsSpec.

It would also be helpful to not log the XML for unmodified items. If it can be implemented easily, the log should show a diff of the XML so that changes are visible.

[leonard84]

Yes, not everyone can do DevOps and even if you can UnitTest the DSLs to find the syntax errors I would see this as an integration test. Both are valuable and should have a different focus.

[arcivanov]

I have it in my queue to work on this this week.

[arcivanov]

@daspilker Quick question - why do you want me to modify the API for createOrUpdateConfigFile, createOrUpdateUserContent and queueJob? This either requires changes in the signatures of all the public APIs and creating overloads for real mode or hundreds of tests have to be modified.

The current implementation assumes that simulation mode is specified at the JobManager level and the above methods would infer their behaviors from that. Would that be acceptable?

[arcivanov]

@daspilker is there a problem with a build system not picking up the latest sources from the PR?

:job-dsl-plugin:compileJavawarning: [options] bootstrap class path not set in conjunction with -source 1.6
warning: No SupportedSourceVersion annotation found on org.kohsuke.wpc.ProcessorImpl, returning RELEASE_6.
/scratch/jenkins/workspace/plugins/job-dsl-plugin/job-dsl-plugin/build/generated-src/stubs/javaposse/jobdsl/plugin/InterruptibleJobManagement.java:10: error: InterruptibleJobManagement is not abstract and does not override abstract method simulation() in JobManagement
@groovy.transform.ThreadInterrupt() public class InterruptibleJobManagement
                                           ^
Note: /scratch/jenkins/workspace/plugins/job-dsl-plugin/job-dsl-plugin/build/generated-src/stubs/javaposse/jobdsl/plugin/InterruptibleJobManagement.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 error
2 warnings
 FAILED

arcivanov@432f6c5#diff-307575c50da6b9a9cb9d2e84e7a93297R167

[daspilker]

I would prefer to use dry run instead of simulation mode, because that's an established term. Please change the wording.

The dry run flag is only used in JenkinsJobManagement, so it should not be exposed in JobManagement or AbstractJobManagement. Just add a field and a getter/setter to JenkinsJobManagement. Do not change any signatures.

Do not reorder any imports, keep the diff at a minimum.

[arcivanov]

will do, thanks

[arcivanov]

Yeah, so there are problems with actual diffs: XML is unstable in Jenkins and produces lots of junk like this. I don't think unified diff will be any better.

Which also means that functionality here doesn't work pretty much ever, since for all practical purposes documents are never identical.

Any thoughts as to what would be less verbose than full printout and but still provide useful diff-ish output @leonard84, @AnalogJ @lanwen ?

Diff (#14): text value
Old node:  <entry ...>
          </entry> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[3]/text()[3]
Old value: 

New node:  <entry ...>
                    </entry> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[3]/text()[3]
New value: 


Diff (#14): text value
Old node:  <targets ...>
          </targets> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/text()[4]
Old value: 

New node:  <targets ...>
                    </targets> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/text()[4]
New value: 


Diff (#14): text value
Old node:  <entry ...>
            </entry> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[4]/text()[1]
Old value: 

New node:  <entry ...>
                        </entry> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[4]/text()[1]
New value: 


Diff (#14): text value
Old node:  <hudson.plugins.cobertura.targets.CoverageMetric ...>METHOD</hudson.plugins.cobertura.targets.CoverageMetric> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[4]/hudson.plugins.cobertura.targets.CoverageMetric[1]/text()[1]
Old value: METHOD
New node:  <hudson.plugins.cobertura.targets.CoverageMetric ...>CLASSES</hudson.plugins.cobertura.targets.CoverageMetric> at /project[1]/publishers[1]/hudson.plugins.cobertura.CoberturaPublisher[1]/failingTarget[1]/targets[1]/entry[4]/hudson.plugins.cobertura.targets.CoverageMetric[1]/text()[1]
New value: CLASSES

[lanwen]

Can you exclude empty New value: (Old value: etc) from output?

Think this enough to understand the main changes

[arcivanov]

@lanwen Those are not empty values, those are multi-line whitespace strings that I forgot to quote (I'll fix it). I can try to diff ignoring whitespace, but that may produce non-deterministic results, I guess.

Any other ideas for diff methodologies?

[leonard84]

Setting XMLUnit to ignore whitespace might be a first step (from Stackoverflow XMLUnit.setIgnoreWhitespace(true))

[arcivanov]

@leonard84 :) I am aware of how to use that library.

Ignoring whitespace, however, may cause functional problems for DSL plugin itself, as whitespace may or may not be functionally significant (who knows what any particular plugin does or needs) and XMLUnit may return true from Diff.isIdentical() where there are substantive changes.

Also, as I pointed out, Jenkins configuration files seem not be order-stable (due to people using HashMaps instead of LinkedHashMaps for data storage, I guess) resulting in variable order of elements that is not functionally significant but does pop up in the diffs creating a mess.

@daspilker thoughts?

[leonard84]

@arcivanov Maybe we should split the diffing from this commit into an enhanced version, a simple smoke test style dry-run would be good enough for me. And the diff would also be interesting for the normal mode, so it would make sense to put it into a separate issue/pr.

[arcivanov]

Makes sense. I am good with simple config dumps as well.
I'll publish the changes with no diffs for now then.

[daspilker]

It would be better to just skip both for loops. As far as I see, in case of a dry run these loops have no effect.

[arcivanov]

I'd like to keep dry-run code as close as possible to the one that would be executed if it was actually running, if it's ok with you.

[daspilker]

Re XML order stability: the DSL should produce identical XML in two identical runs (except for some elements which use a random number), so there should be no problem. If that's not the case, we should fix that (in another PR).

[daspilker]

I played around with computing diffs and I understand the problems better now. See https://github.com/daspilker/job-dsl-plugin/commit/0c8737be84693abfab2333d29d07db711beb1550 on https://github.com/daspilker/job-dsl-plugin/tree/verbose.

I would like to move the logging to a separate pull request and deal with the problems there. Can you remove the logging stuff from this PR?

And add some tests to ExecuteDslScriptsSpec to the test dry run mode changes for ExecuteDslScripts.

[arcivanov]

What do you mean by "move the logging to a separate pull request"? The whole "dry run" point is to be able to see what is generated without actually saving the XML files. If the logging is removed how would the generated content be inspected?

[arcivanov]

@daspilker ping?

[leonard84]

Too bad that it wasn't included in 1.40 release.

[arcivanov]

@leonard84 sorry, but I don't understand what @daspilker wants from this commit anymore. Without logging dry run mode is useless as you can't see what was actually generated, so I'm standing by for more instructions.

[leonard84]

@arcivanov well I would be fine with checking if there are any warnings, e.g. missing/wrong plugin version, wrong credentials id, and so on. Which should mark the dry-run as unstable.

[daspilker]

There are still several issues with this pull request.

• Logging the config XML to the console output may disclose sensitive information. E.g. the XML can contain passwords or other credentials which should not be logged. Logging is not essential for a dry run and should be moved to another pull request which can then deal with the details.
• Executing a script with dry run enabled may fail, but succeed when running the same script with dry run disabled. Run the following examples on a clean installation:

folder('foo')

job('foo/bar')

customConfigFile('myCustomConfigFile') {
  content('test')
}

job('example') {
    wrappers {
        configFiles {
            file('myCustomConfigFile') {
                variable('CONFIG_FILE')
            }            
        }
    }
}

The problem is that the folder and config file are not created, but are checked for existence when validating the job creation.

• DSL scripts can contain arbitrary code. This code may have side effects, e.g. creating files, sending some data to a REST service. These side effects may be undesirable when performing a dry run. It may be a good idea to inject a DRY_RUN variable into the script binding so that scripts can detect a dry run and act accordingly. This should also be mentioned in the help text.
• Tests for ExecuteDslScripts are missing. There should be test cases for the examples that I mentioned above, for the DRY_RUN variable and test that make sure that the descriptor is not modified.

[mmitche]

On my CI instance, I solved this with a different method. I create a folder and generate the CI scripting relative to that folder. Then I call the generator again with no script, which disables all the jobs. This works quite well and maintains the permissions that each job would get generated with. The only issue in this case is that newly generated jobs could start running before they get disabled. I have some changes i the work for this (automatic disabling of jobs)

[warandpeace]

+1 for this feature

[ronnbcnews]

+1 for this feature

[eflamoe]

+1 for this feature.

[Bonko]

+1 for this feature

[waleedali]

+1 cc @arcivanov

[gileshinchcliff]

+1 for this feature

[kvendingoldo]

+1 cc @arcivanov

[sgtcoolguy]

This is really a glorified +1, but I think that the move to Jenkins pipelines actually makes this more relevant. I'd love the ability to have PRs on my set of job dsl scripts where I can do some actual validation of them without trying to set up some large dev environment. Specifically I'd really like to do some basic syntax/linting of the groovy scripts and some sanity check through the DSL plugin using a dry-run mode. Without that, it doesn't make any sense to enforce PRs or anything on these scripts - the only way to "test" them is to commit and see what happens, or to try and set up another Jenkins instance in a VM that we run it on.

[Tin-Nguyen]

+1

[markltbaker]

@daspilker If the pull request was cleaned up with the actions you suggested way back on 13 Nov 2015 would this be acceptable to submit? (turn off logging, add DRY_RUN variable, add ExecuteDslScripts tests)

[CorevistCI]

Build finished.

[red8888]

Any update on this? I want to be able to execute a seed job, have it output to the console what it is going to do, and then wait for user input to confirm- any way of doing that?

[arseniybanayev]

Still looking for this in 2020

[leonard84]

With the addition of https://github.com/jenkinsci/job-dsl-plugin/wiki/Dynamic-DSL (which in itself is a cool feature) and closing issues like https://issues.jenkins.io/browse/JENKINS-55379 with the reason that it is already supported by the dynamic dsl, having a dry-run is a must.

The dynamic DSL is only supported when running in Jenkins, e.g. it is not available when running from the command line or in the Playground.

@daspilker:

As I commented on the JIRA ticket, I still think that having unit test style tests is the better option. They can be run locally by each developer, even from the IDE, in a few seconds. The simulation mode would require to commit the changes and then wait for the CI, fix any problems, commit again and so on. That does not sound like a convenient development cycle.

Well, if we want to use Dynamic-DSL then unit testing is not possible anymore. The new cycle is to commit and fail the seed job which is much worse then testing the new stuff on a feature branch and having a dry-run.

Another alternative would be to generate the DSL API classes on the fly, like it is already done for the API viewer, and let us download a .jar with the current API.

[chitran96]

+1

[felipecrs]

Is there something we can do to help this PR get merged?