add readYaml step #17
Conversation
|
:-( How re-launch Jenkins job? |
| /* | ||
| * The MIT License (MIT) | ||
| * | ||
| * Copyright (c) 2016 CloudBees Inc. |
There was a problem hiding this comment.
I can't find you in the CB directory, do you work for CloudBees? If not then you can't claim that CB owns the copyright, you or the company you work for does ;)
There was a problem hiding this comment.
Sorry for copy/paste... I will put my name
| @@ -33,6 +34,8 @@ | |||
| import org.junit.Test; | |||
| import org.jvnet.hudson.test.JenkinsRule; | |||
|
|
|||
| import hudson.model.Label; | |||
There was a problem hiding this comment.
Can't find any usage of this imported class in your change.
There was a problem hiding this comment.
OK I will clean imports
| @@ -108,6 +108,11 @@ | |||
| <artifactId>maven-model</artifactId> | |||
| <version>3.3.9</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.yaml</groupId> | |||
| <artifactId>snakeyaml</artifactId> | |||
There was a problem hiding this comment.
I don't know how common this module is, but there can be issues with other plugins or core providing the same dependency and that could potentially lead to trouble.
If core or any dependant plugin is providing the same you need to shade it or something. For the maven-model dependency my investigation showed that we shouldn't have any dupes, so that was fine. But I don't know about yaml.
There was a problem hiding this comment.
I can shade it using this maven plugin https://maven.apache.org/components/plugins/maven-shade-plugin/
There was a problem hiding this comment.
Maven Shade Plugin doesn't work due to this Maven HPI Plugin bug: https://issues.jenkins-ci.org/browse/JENKINS-31133 ...
There was a problem hiding this comment.
OK, so we should at least try to estimate if there would be any impact if we include it as is. Normally the dependencies will only clash if the lib is also in core or any plugins that this plugin depends on has it as well. If that's not the case then it should be safe to add as is.
There was a problem hiding this comment.
| if (!StringUtils.isBlank(step.getText())) { | ||
| yamlText += "\n\n" + step.getText(); | ||
| } | ||
| Iterable<Object> yaml = new Yaml().loadAll(yamlText); |
There was a problem hiding this comment.
I tried to follow the code a bit in there, but do you know what type of objects are constructed here? Everything used in the pipeline needs to be Serializable, hence all the hacks in readManifest for example.
Also if there are any custom classes, they also needs to be white listed for the sandbox like in readMavenPom
There was a problem hiding this comment.
To limit objects to standard Java objects like List or Long, I will use SafeConstructor provided by SnakeYAML api:
Yaml yaml = new Yaml(new SafeConstructor());
To ensure that data object is serializable, I will add this:
try(ObjectOutputStream out=new ObjectOutputStream(new ByteArrayOutputStream())){
out.writeObject(yaml);
};
A java.io.NotSerializableException will be thrown if it isn't serializable
…st or Long and ensure that result is serializable
Use UTC timezone to format dates in Junit tests
|
Sorry it took so long |
|
That vendoring is going to need a fix, because snakeyaml depends on Also, given https://issues.jenkins-ci.org/browse/JENKINS-31133 is now fixed AFAIU, I think this could be already converted into an actual shading instead of a vendoring. |
add readYaml step