[JENKINS-42091] Add JSON steps #22

nfalco79 · 2017-02-22T00:02:56Z

The first implementation add a step to read JSON from text or file.
It adds also unit test but with the actual pom.xml there is no way that is work on any OS (Windows 7/MacOS/Linux CentoOS). I have test and write tests with an updated pom.xml

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.jenkins-ci.plugins</groupId>
        <artifactId>plugin</artifactId>
        <version>2.21</version>
    </parent>
    <artifactId>pipeline-utility-steps</artifactId>
    <packaging>hpi</packaging>
    <version>1.2.3-SNAPSHOT</version>
    <name>Pipeline Utility Steps</name>
    <url>https://wiki.jenkins-ci.org/display/JENKINS/Pipeline+Utility+Steps+Plugin</url>

    <licenses>
        <license>
            <name>MIT License</name>
            <url>http://opensource.org/licenses/MIT</url>
        </license>
    </licenses>

    <developers>
        <developer>
            <id>rsandell</id>
            <name>Robert Sandell</name>
            <email>rsandell@cloudbees.com</email>
            <roles>
                <role>maintainer</role>
            </roles>
        </developer>
    </developers>

    <properties>
        <jenkins.version>1.651.3</jenkins.version>
        <workflow.version>1.11</workflow.version>
        <findbugs.failOnError>false</findbugs.failOnError>
    </properties>

    <repositories>
        <repository>
            <id>repo.jenkins-ci.org</id>
            <url>https://repo.jenkins-ci.org/public/</url>
        </repository>
    </repositories>

    <dependencies>
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>script-security</artifactId>
            <version>1.17</version>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-step-api</artifactId>
            <version>${workflow.version}</version>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-cps</artifactId>
            <version>${workflow.version}</version>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-aggregator</artifactId>
            <version>${workflow.version}</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-aggregator</artifactId>
            <classifier>tests</classifier>
            <version>${workflow.version}</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins.workflow</groupId>
            <artifactId>workflow-step-api</artifactId>
            <classifier>tests</classifier>
            <version>${workflow.version}</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.apache.maven</groupId>
            <artifactId>maven-model</artifactId>
            <version>3.3.9</version>
        </dependency>
        <!-- Remove ClassNotFoundException: org.jenkinsci.main.modules.sshd.SshCommandFactory in Junit tests -->
		<dependency>
			<groupId>org.jenkins-ci.modules</groupId>
			<artifactId>sshd</artifactId>
			<version>1.7</version>
			<scope>test</scope>
		</dependency>
    </dependencies>

    <distributionManagement>
        <repository>
            <id>maven.jenkins-ci.org</id>
            <url>https://repo.jenkins-ci.org/releases</url>
        </repository>
    </distributionManagement>

    <scm>
        <connection>scm:git:git://github.com/jenkinsci/pipeline-utility-steps-plugin.git</connection>
        <developerConnection>scm:git:git@github.com:jenkinsci/pipeline-utility-steps-plugin.git</developerConnection>
        <url>https://github.com/jenkinsci/pipeline-utility-steps-plugin</url>
        <tag>HEAD</tag>
    </scm>

    <pluginRepositories>
        <pluginRepository>
            <id>repo.jenkins-ci.org</id>
            <url>https://repo.jenkins-ci.org/public/</url>
        </pluginRepository>
    </pluginRepositories>
</project>

nfalco79 · 2017-02-22T00:26:14Z

Seems that only Cloudbees setup has no problem with current pom.xml and the createOnlineSlave statement of JenkinsRules.
In all environments I have run maven build the creation of slave blocks the maven process indefinitely.

rsandell · 2017-02-22T10:09:13Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStepExecution.java

+            } else if (f.isDirectory()) {
+                logger.print("warning: ");
+                logger.print(f.getRemote());
+                logger.println(" is a directory, omitting from properties gathering");


properties gathering?

copy / paste error. Noted

rsandell · 2017-02-22T10:13:43Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep.java

+    }
+
+    @Extension
+    public static class JSONWhiteLister extends Whitelist {


It's better to contribute this whitelisting to script-security-plugin

This whitelist only permit the basic isArray and size methods needed to navigate json object in pipeline. The json implementation I decide to use (instead GJSON or similar) is because is shipped by default with Jenkins.
In script-security-plugin I see only JVM or groovy library packages so is the right place for net.sf.json? Question because the step readMavenPOM have a more permissive whitelist. Are you moving all custom whitelist to that plugin?

net.sf.json is part of Jenkins core, that's why they belong there. The whitelisting of the maven parts in this plugin is because it is this plugin that adds the dependency.

ok I will create a PR to that plugin refering this discussion.

rsandell · 2017-02-22T10:19:09Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStepExecution.java

+        PrintStream logger = listener.getLogger();
+        JSON json = new JSONObject();
+
+        JsonSlurper jsonSlurper = new JsonSlurper();


Why are you using JsonSlurper? Normally you would just call JSONObject.fromObject(jsonString).

because I do not know if json is a json array or a json object.
By default I setup json as empty json object {} than JsonSlurper instantiates the correct JSONObject or JSONArray.
JsonSluper provides a default jsonConfig and parse from inputstream.

rsandell · 2017-02-22T10:20:55Z

src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadJSONStepTest.java

+        content.put("tags", tags);
+        File file = temp.newFile();
+        try (FileWriter f = new FileWriter(file)) {
+            new JsonBuilder(content).writeTo(f);


content.write(f) no need for JsonBuilder.

nfalco79 · 2017-02-22T12:38:46Z

Test will not pass until this pr is accepted and released

nfalco79 · 2017-02-22T19:30:24Z

Since the custom @Extension of Whitelist no longer exists, it makes no sense to test it here.

If you agree I could proceed to implement writeJSON step

rsandell · 2017-02-23T12:28:02Z

Regarding the pom file problems; have you setup your settings.xml as described here?

nfalco79 · 2017-02-23T12:43:53Z

No but in the pom.xml there are both repository and pluginrepository so the same of settings.
If you update parent you don't need anymore define local plugins because are already defined in parent 2.21
The main issue is that Jenkins core 1.609 is too old and did not run any test.
With 1.651.3 work only local pipeline node() {}, no jenkins client otherwise test hangs (node('client') {})

nfalco79 · 2017-02-27T23:45:11Z

I've add writeJSON step, remove JsonSlurper as requested.
I've also move validation checks to step executor like does the others utility steps (exect readproperties).
Move some UI messages to the messages.properties to be ready for i18n.

If you agree I could also make a single squash commit

Enable i18n for UI messages. Move validation checks to StepExecution instead on the step definition.

rsandell · 2017-02-28T11:31:13Z

script-security-plugin #109 was released in script-security-1.27 so you can probably bump the dependency to make the tests pass If you want to tests those bits.

rsandell · 2017-02-28T11:36:42Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStepExecution.java

+        if (step.getJson() == null) {
+            throw new IllegalArgumentException(Messages.WriteJSONStepExecution_missingJSON(step.getDescriptor().getFunctionName()));
+        }
+        if (StringUtils.isBlank(step.getFile())) {


Since you do require both arguments to be set they should both be in the @DataboundConstructor.

right, I miss due I've add json later.

rsandell · 2017-02-28T11:37:32Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStepExecution.java

+/*
+ * The MIT License (MIT)
+ *
+ * Copyright (c) 2016 CloudBees Inc.


Do you work for CloudBees? If not you shouldn't claim their copyright.

I didn't comment on the copyright of the other step's files since they where about 90% copy & paste so I deemed that to be correct.

I do not work for CloudBees so what I could wrote? Should I have to remove Copyright row?

Either you own the copyright or the company you work for.

rsandell · 2017-02-28T11:40:35Z

src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStep/config.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+    <f:block>
+        <p>This is a special step. No snippet generation available. See inline help for more information.</p>


I think you can make the snippet generator work here since you are using standard data bound fields.

this means that I can remove both config.jelly and help.html files?

No, you should change config.jelly to contain a proper config page for the databound fields. And change the help file to more explain to a human what the step actually does.

Snippet generator is not able to handle correctly complex type like net.sf.json.JSON (interface) in data bound fields. So I leave the same as maven step does where the help explain how to use by example.

Instead readJSON config.jelly was changed to support snippet generator also with optional fields. The same could be do for readProperties and readManifest.

Yes, sorry, I was stupid.

rsandell · 2017-02-28T11:42:46Z

src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/json/WriteJSONStepTest.java

+        WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition(
+                "node {\n" +
+                        "  def json = readJSON file: '" + separatorsToSystemEscaped(file.getAbsolutePath()) + "'\n" +


I think we need to test that non system escaped separators works. Java should translate some/path to some\path on windows without us needing to do anything. If that doesn't work then we need to fix that in the step, the users shouldn't have to do that.

It's one of the main points of this plugin to be platform agnostic.

ok, consider that all yaml, properties, maven, manifest (where I copy test methodology) use same escape function. I will change it

Yes I was a bit out of touch when I approved those changes. I think the problem they solved was that on windows the path would be 'some\path' when calling file.getAbsolutePath() and that caused issues.
What I should have argued in that case was to make sure the format was always `some/path'.
So keep it like it is in here and I'll fix it in another PR at a later date.

Move pipeline-security version to 1.27. Mark fields of writeJSON step as required. Reduce use of real file in test case.

nfalco79 · 2017-02-28T22:17:50Z

Fix copyright header.
Move pipeline-security version to 1.27.
Mark fields of writeJSON step as required.
Reduce use of real file in test case and use path separator /.
Remove config.jelly to use automatic ?snippet generator?

Let me squash commits if pull request could be considered approved by you

rsandell

Missing config.jelly with data bindings fir writeJSON

…teps

nfalco79 · 2017-03-05T11:27:45Z

The Snippet generator does not handle correctly any FormException("message", "fieldName"), the only checked exception of newInstance(StaplerRequest req, JSONObject formData).

I would notice that in Jenkins 1.x any exception server side is showed in the snippet text area as trackstrace instead in Jenkins 2.x no error is shown at all. I would expect that form exception message was placed near the form exception field.

rsandell · 2017-03-06T12:26:13Z

Ah, yes. I had a temporary lapse in stupidness, sorry :)

rsandell

Seems to suffer from more copy paste problems.

rsandell · 2017-03-06T12:28:26Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStepExecution.java

+            FilePath f = ws.child(step.getFile());
+            if (f.exists() && !f.isDirectory()) {
+                try (InputStream is = f.read()) {
+                    json = JSONSerializer.toJSON(IOUtils.toString(is));


You can return the object here directly, since you don't seem to try to do any override (even if your help text below at times indicates you do)

I could but personally I do not like too much exit point, when is possible I prefer this way

SonarQube open a violation when >= 5 return in a method

OK, it's just that this behaves a bit differently then from what is stated, if both file and text is set (which is blocked in newInstance but can happen by other means) it would read the file and then overwrite the val with what is in text. Nothing wrong with that, just that the code doesn't fully reflect what is expected.

If both fields are set an IllegalArgumentException will be throw at the begin of this method

rsandell · 2017-03-06T12:28:37Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStepExecution.java

+            }
+        }
+        if (!isBlank(step.getText())) {
+            json = JSONSerializer.toJSON(step.getText().trim());


rsandell · 2017-03-06T12:31:03Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStepExecution.java

+        if (path.isDirectory()) {
+            throw new FileNotFoundException(Messages.JSONStepExecution_fileIsDirectory(path.getRemote()));
+        }
+


Perhaps a nice thing to do would be to warn if you are overwriting an existing file.

Typicall scenario of writeJSON is read json file and manipulate its data, add other data and update the same file (like maven step does).
For example autoincrement version: beta3 -> beta4 of a package.json
If I put a warn it's very annoing in console log.

I could add a boolean property override to step, but this could be a show stopper in case devops change the pipeline and/or developer would provide a base JSON file to be enhance with writeJSON step, etc etc.

rsandell · 2017-03-06T12:31:42Z

...main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep/help-file.html

+  ~ SOFTWARE.
+  -->
+<p>
+	Path to a file in the workspace to read the properties from.


Properties?

rsandell · 2017-03-06T12:32:09Z

...main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep/help-file.html

+  -->
+<p>
+	Path to a file in the workspace to read the properties from.
+	<em>These are added to the resulting map after the defaults and so will overwrite any key/value pairs already present.</em>


rsandell · 2017-03-06T12:34:29Z

...main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep/help-text.html

+  ~ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+  ~ SOFTWARE.
+  -->
+<p>A JSON object data, written as name/value pairs. Values must be


JSON data is normally structured and I don't see any indications of you limiting that structure to be name/value pairs.
Shouldn't just "A string containing JSON formatted data" or similar be enough?

rsandell · 2017-03-06T12:38:25Z

src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep/help.html

+<p>
+    <strong>Example:</strong><br/>
+    <code><pre>
+def props = readJSON file: 'dir/input.json', text: '{ other: Override }'


In help-text.html and help-file.html you state that file or text can be used, not both at the same time. Yet this example uses both.
Also I don't think the text will parse since Override isn't surrounded by quotes.

The use of Override is also a bit confusing, since the other steps in this plugin that uses it to exeplify how to override data in the file. So it's better to use some other string to not further confuse the user.

the example is wrong, too many copy/paste in late hours

rsandell · 2017-03-06T12:38:59Z

src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStep/config.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+    <f:block>
+        <p>This is a special step. No snippet generation available. See inline help for more information.</p>


Yes, sorry, I was stupid.

[JENKINS-42091] Add readJSON step implementation

e55530e

nfalco79 changed the title ~~[JENKINS-42091] Add JSON steps to read and write~~ [JENKINS-42091] Add step to read JSON Feb 22, 2017

[JENKINS-42091] Update documentation

ff194ee

rsandell requested changes Feb 22, 2017

View reviewed changes

[JENKINS-42091] Changes for code review approval

43a0bea

nfalco79 mentioned this pull request Feb 22, 2017

[JENKINS-42091] Add some needed methods of JSON superclass jenkinsci/script-security-plugin#109

Merged

[JENKINS-42091] Remove isArray() and size() from unit test.

cf048f5

[JENKINS-42091] Add writeJSON step.

2dc5eb6

Enable i18n for UI messages. Move validation checks to StepExecution instead on the step definition.

rsandell requested changes Feb 28, 2017

View reviewed changes

[JENKINS-42091] Fix copyright header.

37f396f

Move pipeline-security version to 1.27. Mark fields of writeJSON step as required. Reduce use of real file in test case.

nfalco79 changed the title ~~[JENKINS-42091] Add step to read JSON~~ [JENKINS-42091] Add JSON steps Mar 1, 2017

rsandell requested changes Mar 3, 2017

View reviewed changes

[JENKINS-42091] Add the right config jelly for both read/write JSON s…

3b916c8

…teps

rsandell requested changes Mar 6, 2017

View reviewed changes

[JENKINS-42091] Fix help inline

faf7586

rsandell approved these changes Mar 7, 2017

View reviewed changes

rsandell merged commit 9e19d2d into jenkinsci:master Mar 7, 2017

[JENKINS-42091] Add JSON steps #22

[JENKINS-42091] Add JSON steps #22

Conversation

nfalco79 commented Feb 22, 2017 • edited

nfalco79 commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 Feb 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 commented Feb 22, 2017 • edited

nfalco79 commented Feb 22, 2017

rsandell commented Feb 23, 2017

nfalco79 commented Feb 23, 2017 • edited

nfalco79 commented Feb 27, 2017 • edited

rsandell commented Feb 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 commented Feb 28, 2017

rsandell left a comment

Choose a reason for hiding this comment

nfalco79 commented Mar 5, 2017

rsandell commented Mar 6, 2017

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 Mar 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 commented Feb 22, 2017 •

edited

nfalco79 Feb 22, 2017 •

edited

nfalco79 commented Feb 22, 2017 •

edited

nfalco79 commented Feb 23, 2017 •

edited

nfalco79 commented Feb 27, 2017 •

edited

rsandell commented Feb 28, 2017 •

edited

nfalco79 Mar 7, 2017 •

edited