[JENKINS-50636] Use @DataBoundSetter for optional pipeline params by rmstyrczula · Pull Request #48 · jenkinsci/plot-plugin

rmstyrczula · 2018-05-28T21:13:41Z

This helps the Snippet Generator provide a smaller example
where many defaults are used, and prevents the Jenkins pipeline
linter from complaining about missing "required" parameters.

JIRA

No issue directly found, though the closest I could find is JENKINS-50636.

Misc

I started working on this due to build failures in my Jenkins pipeline when using a Jenkinsfile from SCM. This does not seem to happen with an inline pipeline job.

plot-build-failure.txt

What has been done

Optional constructor parameters have been removed and have been given @DataBoundSetters
Generic series has been removed and csv, xml, properties are concatenated during perform.

How to test

You can check if a Jenkinsfile will pass muster by using the built-in linter

The following Jenkinsfile works with my changes, but fail with errors similar to the attached plot-build-failure.txt.

(You can also just create a simple Pipeline job and paste the following)

pipeline {
  agent any
  stages {
    stage('write-data') {
      steps {
        sh '''
          echo '<my_data>' > data.xml
          echo '    <test value1="123.456"/>' >> data.xml
          echo '    <test value2="321.654"/>' >> data.xml
          echo '</my_data>' >> data.xml
        '''
      }
    }
  }
  post {
    always {
      plot csvFileName: 'plot-d519ae37-01b4-446b-a901-a98d0b28efe6.csv',
           group: 'My Data',
           title: 'Useful Title',
           style: 'line',
           yaxis: 'arbitrary',
           xmlSeries: [
             [file: 'data.xml',
              nodeType: 'NODESET',
              xpath: 'my_data/test/@*']
           ]
    }
  }
}

I tested this with Jenkins (LTS 2.107.3) inside Docker:

docker run -d --name jenkins -p 127.0.0.1:8080:8080 -p 50000:50000 -v jenkins_home:/var/jenkins_home jenkins/jenkins:lts

I installed the suggested plugins, and then uploaded the built hpi to test.

Checklist

Git commits follow best practices
Build passes in Jenkins
Appropriate tests or explanation to why this change has no tests
JIRA issue is well described (problem explanation, steps to reproduce, screenshots)
For dependency updates: links to external changelogs and, if possible, full diffs

vgaidarji · 2018-05-29T10:33:08Z

+
+    // Generated?
    @SuppressWarnings("visibilitymodifier")
    public String csvFileName;


Probably this field should be moved to the top where other Required fields are located.

This one is a little odd, since it's not directly specified by the user with the snippet generator. I'm assuming it's generated here through reflection when the step is created.

No need to move, your assumption is correct. Maybe, in this case, explanation comment from where it comes will be better than Generate?.

vgaidarji · 2018-05-29T10:45:02Z

        return yaxisMaximum;
    }

-    public List<Series> getSeries() {


Verified that this one was actually never called from outside. Should be safe to remove.

I'm not super happy with the removal of this, as it splits the pipeline and freestyle jobs further from each other. I tried taking parts of the freestyle config.jelly, unfortunately it's not just simple replacement. I'm not familiar enough with freestyle jobs to say why that is.

vgaidarji · 2018-06-01T08:42:32Z

@rmstyrczula would be nice to have tests for different combinations.
It can be tricky, since it requires base Jenkins version update. But I guess most of the plugin users who use it in pipeline have their Jenkins more or less up to date (need to prove this somehow).

I've prepared a patch for you with a working test and pipeline you provided (it's a .patch file inside, GitHub doesn't support those formats, had to wrap into .zip)
0001-Add-tests.patch.zip
I've update the base Jenkins from 1.625.3 to 2.60.3. Would be nice to find minimum supported version where tests will work. Tests were failing for me with 1.625.3 because of missing class - http://javadoc.jenkins.io/jenkins/model/OptionalJobProperty.html. According to the docs it was added in 1.637 version. But there can be other dependencies as well. Would be nice if you can investigate that. Or at least provide required tests and then I can play myself with dependencies versions.

We need to have tests (at least):

with mandatory parameters only
test for XML series
test for Properties series
test for CSV series
...

vgaidarji · 2018-06-01T08:46:07Z

@rmstyrczula + would be nice to include JIRA ticket id in a commit message (check this PR for example). This will post updates to JIRA ticket when PR get's merged.

rmstyrczula · 2018-06-01T13:40:03Z

I'll look into adding your patched tests and if I can add my own.

Regarding the JIRA ticket, do you want me to reference JENKINS-50636?

edit: There is a deprecated jenkins image with many older tags. I'll check that out to test older versions

rmstyrczula · 2018-06-02T21:44:04Z

Through testing, it seems 2.0 is the minimum required version now. Testing on even the latest 1.x version, 1.656, has failures with the new PlotBuilder tests.

I added your patch, added some basic tests on top of it, and modified the original commit message to include the JIRA ticket ID.

Let me know if I've missed anything!

vgaidarji · 2018-06-03T06:21:07Z

@rmstyrczula good job! That's exactly what I've expected from those tests.
Regarding Jira ticket, could you please format them in a following way

JENKINS-50636 Use @DataBoundSetter to support optional pipeline params
JENKINS-50636 Configure environment for PlotBuilder pipeline tests

Divide 10d5c29 commit into 2 (if tricky, modifying commit message should be enough):

JENKINS-50636 Reduce required Jenkins version (with detailed explanation why we changed the Jenkins version)
JENKINS-50636 Cover PlotBuilder with tests

I've tested on freestyle/matrix/pipeline with parameters/pipeline with minimal required parameters jobs and everything works as expected 👍

Once you fix the commit messages, I'll merge and release this fix. Thank you so much!

This helps the Snippet Generator provide a smaller example where many defaults are used, and prevents the Jenkins pipeline linter from complaining about missing "required" parameters.

Pipeline support requires a minimum of 2.0 for newer Jenkins API functionality for subsequent unit tests.

rmstyrczula · 2018-06-04T01:53:34Z

I split out the pom update to its own commit, and put your patched tests plus the others in the last commit. Changed the wording as you requested.

vgaidarji · 2018-06-04T12:11:29Z

@rmstyrczula thank you so much for this fix 🎉.
2.1.0 version released (link to changelog). New version will be available soon via update center.

vgaidarji self-assigned this May 31, 2018

vgaidarji reviewed Jun 1, 2018

View reviewed changes

rmstyrczula force-pushed the pipeline-setters branch from 1a7a8de to 10d5c29 Compare June 2, 2018 21:38

vgaidarji changed the title ~~Use @DataBoundSetter for optional pipeline params~~ [JENKINS-50636] Use @DataBoundSetter for optional pipeline params Jun 3, 2018

qrsty and others added 3 commits June 3, 2018 21:37

JENKINS-50636 Use @DataBoundSetter to support optional pipeline params

de2457b

This helps the Snippet Generator provide a smaller example where many defaults are used, and prevents the Jenkins pipeline linter from complaining about missing "required" parameters.

JENKINS-50636 Update required Jenkins version

00da15d

Pipeline support requires a minimum of 2.0 for newer Jenkins API functionality for subsequent unit tests.

JENKINS-50636 Cover PlotBuilder with tests

6b0d3d2

rmstyrczula force-pushed the pipeline-setters branch from 10d5c29 to 6b0d3d2 Compare June 4, 2018 01:52

vgaidarji merged commit 052beb6 into jenkinsci:master Jun 4, 2018

rmstyrczula deleted the pipeline-setters branch June 4, 2018 14:18

Uh oh!

Uh oh!

Conversation

rmstyrczula commented May 28, 2018 • edited by vgaidarji Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Misc

What has been done

How to test

Checklist

Uh oh!

vgaidarji May 29, 2018

Choose a reason for hiding this comment

Uh oh!

rmstyrczula Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

vgaidarji Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

vgaidarji May 29, 2018

Choose a reason for hiding this comment

Uh oh!

rmstyrczula Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

vgaidarji commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgaidarji commented Jun 1, 2018

Uh oh!

rmstyrczula commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmstyrczula commented Jun 2, 2018

Uh oh!

vgaidarji commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmstyrczula commented Jun 4, 2018

Uh oh!

vgaidarji commented Jun 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rmstyrczula commented May 28, 2018 •

edited by vgaidarji

Loading

vgaidarji commented Jun 1, 2018 •

edited

Loading

rmstyrczula commented Jun 1, 2018 •

edited

Loading

vgaidarji commented Jun 3, 2018 •

edited

Loading