Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented writeYaml step #23

Merged
merged 8 commits into from
Aug 4, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/STEPS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* `readProperties` - Read [java properties](https://docs.oracle.com/javase/7/docs/api/java/util/Properties.html) from files in the workspace or text. ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadPropertiesStep/help.html))
* `readManifest` - Read a [Jar Manifest](https://docs.oracle.com/javase/7/docs/technotes/guides/jar/jar.html#JAR_Manifest). ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/mf/ReadManifestStep/help.html))
* `readYaml` - Read [YAML](http://yaml.org) from files in the workspace or text. ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadYamlStep/help.html))
* `writeYaml` - Write a [YAML](http://yaml.org) file from an object. ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep/help.html))
* `readJSON` - Read [JSON](http://www.json.org/json-it.html) from files in the workspace or text. ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/ReadJSONStep/help.html))
* `writeJSON` - Write a [JSON](http://www.json.org/json-it.html) object to a files in the workspace. ([help](../src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/json/WriteJSONStep/help.html))

Expand Down
9 changes: 8 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.27</version>
<version>1.29.1</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -175,4 +175,11 @@
<url>https://github.com/jenkinsci/pipeline-utility-steps-plugin</url>
<tag>HEAD</tag>
</scm>

<pluginRepositories>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time for a new parent pom before trying to fix this.

<pluginRepository>
<id>repo.jenkins-ci.org</id>
<url>http://repo.jenkins-ci.org/public/</url>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https 🐋

</pluginRepository>
</pluginRepositories>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
/*
* The MIT License (MIT)
*
* Copyright (c) 2017 Javier DELGADO
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/

package org.jenkinsci.plugins.pipeline.utility.steps.conf;

import hudson.Extension;
import hudson.FilePath;
import hudson.model.TaskListener;
import org.jenkinsci.plugins.pipeline.utility.steps.shaded.org.yaml.snakeyaml.DumperOptions;
import org.jenkinsci.plugins.pipeline.utility.steps.shaded.org.yaml.snakeyaml.Yaml;
import org.jenkinsci.plugins.workflow.steps.*;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.Nonnull;
import javax.inject.Inject;
import java.io.*;
import java.net.URL;
import java.nio.file.FileAlreadyExistsException;
import java.util.*;

import static org.apache.commons.lang.StringUtils.isBlank;

/**
* Writes a yaml file from the workspace.
*
* @author Javier DELGADO &lt;witokondoria@gmail.com&gt;.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since TODO would be useful since we make attempts to visualize extension javadocs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed. it's not present in any other of the steps.

public class WriteYamlStep extends AbstractStepImpl {

private String file;
private Object data;

@DataBoundConstructor
public WriteYamlStep(@Nonnull String file, @Nonnull Object data) {
if (file == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.isBlank()

throw new IllegalArgumentException("file parameter must be provided to writeYaml");
}
this.file = file;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

truncate spaces?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also make sense to verify the patch and to ensure nobody passes an absolute one or a relative with traversal to upper levels?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to trim the string? Forcing a path from within the current current workspace seems fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the writeFile Step, paths are not verified (so traversal is also possible).

Would you prefer a higer level implementation for this check?

if (data == null) {
throw new IllegalArgumentException("data parameter must be provided to writeYaml");
} else if (!isValidObjectType(data)) {
throw new IllegalArgumentException("data parameter has invalid content (no-basic classes)");
}
this.data = data;
}

/**
* Name of the yaml file to write.
*
* @return file name
*/
public String getFile() {
return file;
}

/**
* Name of the yaml file to write.
*
* @param file file name
*/
public void setFile(String file) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need setters? If no, I would advice to either remove them and make the fields final OR to convert the class to @DataBoundSetters

this.file = file;
}

/**
* An Object containing data to be saved.
*
* @return data to save as yaml
*/
public Object getData() {
return data;
}

/**
* An Object representing the elements to write.
*
* @param data to parse
*/
public void setData(Object data) {
this.data = data;
}

private boolean isValidObjectType(Object obj) {
if ((obj instanceof Boolean) || (obj instanceof Character) ||
(obj instanceof Number) || (obj instanceof String) ||
(obj instanceof URL) || (obj instanceof Calendar) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(obj instanceof Date) || (obj instanceof UUID) ||
(obj == null)) {
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is strange, error-prone (E.g. GString, Long, etc.) and ineffective. Why would you need it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not a complete and final list, but a one less strict than plain Maps, with objects already whitelisted when toJson invocations:
Long is covered by Number
GStrings hasnt been even tested :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that the GStrings will be evaluated/expanded in CPS/sandbox before entering the java context, but it needs to be tested non the less.

something like:

writeYaml data: "${currentBuild.rawBuild}", file: 'data'

might be enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add tests tomorrow!

} else if (obj instanceof Map) {
Boolean ret = true;
Set entry = ((Map) obj).entrySet();
Iterator iterator = entry.iterator();
while (iterator.hasNext() && ret) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit convoluted, something like this would be more readable and simpler I think

for (Map.Entry entry : map.entrySet()) {
    if (!isValidObject(entry.getKey()) || !isValidObject(entry.getValue())) {
        return false;
    }
}

It also seems like you are overwriting the ret var every time in the loop, so effectively you are only checking the last entry in the map which my suggestion also solves.

Copy link
Member Author

@witokondoria witokondoria Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that, Its result of a late commit. Will fix it.

Object next = iterator.next();Object es = ((Map) obj).get(next);
ret = (isValidObjectType(((Map.Entry)next).getKey()) && isValidObjectType(((Map.Entry)next).getValue()));
}
return ret;
} else if (obj instanceof Collection) {
Boolean ret = true;
Iterator iterator = ((Collection) obj).iterator();
while (iterator.hasNext() && ret) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same simplification and issue here as for the Map.
Suggestion:

for (Object o : ((Collection)obj)) {
    if (!isValidObject(o)) {
        return false;
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Object next = iterator.next();
ret = isValidObjectType(next);
}
return ret;
} else {
return false;
}
}

@Extension
public static class DescriptorImpl extends AbstractStepDescriptorImpl {

public DescriptorImpl() {
super(Execution.class);
}

@Override
public String getFunctionName() {
return "writeYaml";
}

@Override
public String getDisplayName() {
return "Write a yaml from an object.";
}
}

public static class Execution extends AbstractSynchronousNonBlockingStepExecution<Void> {
private static final long serialVersionUID = 1L;

@StepContextParameter
private transient TaskListener listener;

@StepContextParameter
private transient FilePath ws;

@Inject
private transient WriteYamlStep step;

@Override
protected Void run () throws Exception {
FilePath path = null;
if (!isBlank(step.getFile())) {
path = ws.child(step.getFile());
if (path.exists()) {
throw new FileAlreadyExistsException(path.getRemote() + " already exist.");
}
if (path.isDirectory()) {
throw new FileNotFoundException(path.getRemote() + " is a directory.");
}
}

DumperOptions options = new DumperOptions();
options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK);
Yaml yaml = new Yaml(options);

try (OutputStreamWriter writer = new OutputStreamWriter(path.write())) {
yaml.dump (step.getData(), writer);
}

return null;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
~ The MIT License (MIT)
~
~ Copyright (c) 2017 Javier DELGADO.
~
~ Permission is hereby granted, free of charge, to any person obtaining a copy
~ of this software and associated documentation files (the "Software"), to deal
~ in the Software without restriction, including without limitation the rights
~ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
~ copies of the Software, and to permit persons to whom the Software is
~ furnished to do so, subject to the following conditions:
~
~ The above copyright notice and this permission notice shall be included in all
~ copies or substantial portions of the Software.
~
~ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
~ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
~ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
~ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
~ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
~ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
~ SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:entry title="${%file.title}" field="file" description="${%file.description}">
<f:textbox />
</f:entry>

<f:entry title="${%text.title}" field="text" description="${%text.description}">
<f:textbox />
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
file.title=File
file.description=File to save as YAML
text.title=Data
text.description=Object to serialize yo YAML
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yo dawg, I heard yo and yo dawg like yo yos.

Copy link
Member Author

@witokondoria witokondoria Jul 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, that made my afternoon

Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<!--
~ The MIT License (MIT)
~
~ Copyright (c) 2017 Javier DELGADO.
~
~ Permission is hereby granted, free of charge, to any person obtaining a copy
~ of this software and associated documentation files (the "Software"), to deal
~ in the Software without restriction, including without limitation the rights
~ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
~ copies of the Software, and to permit persons to whom the Software is
~ furnished to do so, subject to the following conditions:
~
~ The above copyright notice and this permission notice shall be included in all
~ copies or substantial portions of the Software.
~
~ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
~ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
~ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
~ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
~ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
~ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
~ SOFTWARE.
-->
<p>
Writes a yaml file in the current working directory from an Object or a String.
It uses <a href="https://bitbucket.org/asomov/snakeyaml" target="_blank">SnakeYAML</a> as YAML processor.
The call will fail if the file already exists.
</p>
<strong>Fields:</strong>
<ul>
<li>
<code>file</code>:
Mandatory path to a file in the workspace to write the YAML datas to.
</li>
<li>
<code>data</code>:
A Mandatory Object containing the data to be serialized.
</li>
</ul>
<p>
<strong>Examples:</strong><br/>
<code>
<pre>
def amap = ['something': 'my datas',
'size': 3,
'isEmpty': false]

writeYaml file: 'datas.yaml', data: amap
def read = readYaml file: 'datas.yaml'

assert read.something == 'my datas'
assert read.size == 3
assert read.isEmpty == false
</pre>
</code>
</p>
Loading