Make plugin compatible with Workflow #13

martinda · 2016-01-16T04:06:43Z

Support vim files in .gitignore
Deprecated global properties marked to be deprecated
Refactored code as per Workflow Compatibility guide
Fixed serialization NPE
Add TEXT_PLAIN mime type
Use maven-hpi-plugin for built-in plugin tests
Set slf4j-api to 1.7.13 (latest at this time)
Better error message when return code is out of bounds
Set the default timeout value for workflow
Added many tests

martinda · 2016-01-16T04:46:58Z

@janario please review and provide feedback. I would like these changes to be included in the next official release. This pull request prepares the plugin for workflow integration, which will require a new custom build step to be developed in a subsequent pull request.

jenkinsadmin · 2016-01-16T07:04:52Z

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

oleg-nenashev · 2016-01-18T09:23:41Z

pom.xml

@@ -4,7 +4,7 @@
        <groupId>org.jenkins-ci.plugins</groupId>
        <artifactId>plugin</artifactId>

-        <version>1.509.4</version>
+        <version>1.609.1</version>


Needs a review from plugin maintainers. I'm not sure which baselines do they target

1.580.1 would be the usual minimum for Pipeline compatibility (up to 1.4.2). But if the maintainer agrees, this is fine too.

oleg-nenashev · 2016-01-18T09:32:10Z

There're much unrelated formatting and functional changes. Would be great if they go to separate pull-requests or at least commits

martinda · 2016-01-18T15:15:51Z

Thank you for this code review, it helps me improve the code.

There're much unrelated formatting and functional changes. Would be great if they go to separate pull-requests or at least commits

I am not sure how to separate into multiple commits or multiple pull-requests but I will think about it.

I made some changes not because they were absolutely necessary, but because it made it easier for me to implement the Workflow compatibility. As far I can can tell, I have maintained compatibility at the freestyle build level (that's why I wrote so many tests).

As an example, the lack of TEXT_PLAIN mime type was not a bug nor a feature request, but it made it a bit easier to write the tests. Another example is that the author was not using the maven-hpi-plugin, but it was recommended I use it, so I put it in. The slf4j upgrade was not necessary, but along the way I've put it in probably because of something else I tried that needed it. The NPE serialization issue came up, but it has nothing to do with Workflow per say, however, it got in the way at some point, so I fixed it.

The code snippet generator was also very useful. At first it would come up with almost all the arguments as being necessary. So I worked on improving how default arguments and default values came about. Now the snippet generator only show the mandatory arguments, and has default values (rather than null) for the other arguments. From the outside, I guess these "formatting" changes look unnecessary?

I think some of the code formatting comes from breaking the constructor as per the COMPATIBILITY.md recommendations, and placing the setters/getters in the same order as they are in the jelly code. So no matter which file you read, the user parameters are ordered the same way.

martinda · 2016-01-18T19:32:38Z

@oleg-nenashev I have a split this PR into more commits, how would you like me to present them? Should I force update this PR, or create a new PR? Thanks.

Follow the COMPATIBILITY.md guidelines Add public boolean perform(AbstractBuild<?, ?> build, ...) method for non-workflow java compatibility Bug fix: expect entity to be null in response to HEAD method request Fix serialization by removing the logger which could be null and cause NPE Use apache LocalServerTestBase to test request content Support the TEXT_PLAIN mime type to simplify testing Better error message when return code is out of acceptable bounds Build parameters are passed as code when in Workflow Skip outputFile writing when Workspace is null and inform user about it

oleg-nenashev · 2016-01-19T17:07:06Z

@martinda
Force-push to the forked repo with a PR is fine in the case of GitHub

martinda · 2016-01-20T01:22:29Z

@oleg-nenashev the code is ready for more review. I have pushed a short series of commits that attempt to separate changes in a few categories.

amuniz · 2016-01-26T09:36:49Z

src/test/java/jenkins/plugins/http_request/HttpRequestTest.java

+
+        // Check expectations
+        j.assertBuildStatusSuccess(build);
+        String s = FileUtils.readFileToString(build.getLogFile());


Easier (to read and maintain) with j.assertLogContains("All is well", build)

amuniz · 2016-01-26T09:41:20Z

Didn't try to run it, but looks good to me. A couple of tips:

Did you check that the Snippet Generator is working fine (I mean, it generates the correct groovy code - taking care of default values)?
A Pipeline (the new name of Workflow) specific test would be in place (since the aim of the PR is to add Pipeline compatibility). You can find examples in the Pipeline Plugin test code.

jglick · 2016-01-26T12:16:01Z

src/main/java/jenkins/plugins/http_request/HttpRequest.java

+    throws InterruptedException, IOException {
+        perform(build, build.getWorkspace(), launcher, listener);
+        return true;
+    }


No need to retain this overload. The super implementation will call the new one automatically.

jglick · 2016-01-26T12:25:40Z

Mostly OK but deserves a Pipeline-specific functional test, and there are some fixes needed in the databinding (which should anyway make the UI code simpler and more regular).

KostyaSha · 2016-01-27T16:42:15Z

pom.xml

@@ -56,4 +78,16 @@
            <email>janarioliver@gmail.com</email>
        </developer>
    </developers>
+
+    <build>
+        <pluginManagement>


pluginManagment doesn't make any sense in non multi-module project. why do you need it?

@KostyaSha It is to use the maven-hpi-plugin, I was told it would give me some built-in tests. What is a the correct way to use maven-hpi-plugin?

if it works, then ok. I thought that you need to set plugin directly in build tag

I removed the pluginManagement element and it also works, so I am removing it. Thanks.

martinda · 2016-02-22T15:24:17Z

I think this plugin misuses doValidate. There is no validation button, no communication to an external server, no apparent external side-effect. The FormAuthentication.FormAuthenticationDescriptor.doCheckKeyName() even calls HttpRequest.DescriptorImpl.validateKeyName(value).

jglick · 2016-02-22T15:34:09Z

I think this plugin misuses doValidate.

Agreed, from what I can see check seems more appropriate here than validate.

jglick · 2016-02-22T15:35:47Z

Judging by the console log where I sent a SIGQUIT, the build was hanging due to an OOME in PermGen. Switched CI job configuration to use Java 8 in an attempt to avoid that problem.

priytamaj · 2016-03-29T22:53:42Z

Does http-request-plugin support for workflow/pipeline supports REST api invocation using certificates?

omehegan · 2016-04-04T18:21:57Z

Any chance of this being merged soon? Is the code as it stands safe to use, even if not integrated? It sounds like the only issue is with tests timing out. Correct?

martinda · 2016-04-04T18:38:34Z

@omehegan There is an issue with the migration of the global configuration for the plugin. I have reproduced the issue in a separate repository. As soon as that is figured out, I can fix this PR.

I should add that I am not actively working on this particular issue at this time, because I have run out of ideas on what to do. Any help appreciated!

omehegan · 2016-04-04T18:56:50Z

OK. Sounds like if I have never used the plugin before, I could install this branch and be fine? I was looking for this exact functionality!

janario · 2016-04-20T02:14:48Z

Hi guys,

First of all I have to thank you all about the review and suggestions on this PR.
I was in a very busy routine and I wasn't able to see this PR as far as I wanted.

@martinda I really liked the PR, especially about the test cases :)

I have some questions and some suggestions bellow

Guys, jenkins plugin parent 2.3, Will this make the plugin incompatible with old versions of jenkins?

@martinda , you can add @ author tag in all classes you have created. :)
About 'developer' tag in pom.xml. It is meant to be for developers who has commit access and do support.
Can you change it for 'contributor' ? As 'developer' you may receive some email asking for support and feature.

TestCases

Tests cases are failing because of OOME. As I've tested with Xmx320m is good enough. Can you configure it?

<plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-surefire-plugin</artifactId>
        <configuration>
            <argLine>-Xmx320m</argLine>
        </configuration>
</plugin>

Global configuration

As you said, there is a compatibility problem in global configuration.
The persistent file used to be 'jenkins.plugins.http_request.HttpRequest.xml', to keep persisting on it just add the following

public class HttpRequestGlobalConfig extends GlobalConfiguration {
    private static final XStream2 XSTREAM2 = new XStream2();
    @Initializer(before = InitMilestone.PLUGINS_STARTED)
    public static void xStreamCompatibility() {
        XSTREAM2.addCompatibilityAlias("jenkins.plugins.http_request.HttpRequest$DescriptorImpl", HttpRequestGlobalConfig.class);
        XSTREAM2.addCompatibilityAlias("jenkins.plugins.http_request.util.NameValuePair", HttpRequestNameValuePair.class);
    }

    @Override
    protected XmlFile getConfigFile() {
        return new XmlFile(XSTREAM2, new File(Jenkins.getInstance().getRootDir(), "jenkins.plugins.http_request.HttpRequest.xml"));
    }
...
}

Once removed the global deprecated configuration 'defaultHttpMode', 'defaultReturnCodeBuildRelevant', 'defaultLogResponseBody'
I'll release it with break compatibility. This was known since I deprecated them :)

Job configuration
The properties 'validResponseCodes' and 'customHeaders' used to be null on old versions. Which throw NPE breaking compatibility.
You can add the readResolve method on HttpRequest to avoid this.

protected Object readResolve() {
        if (customHeaders == null) {
            customHeaders = DescriptorImpl.customHeaders;
        }
        if (validResponseCodes == null || validResponseCodes.trim().isEmpty()) {
            validResponseCodes = DescriptorImpl.validResponseCodes;
        }
        return this;
}

And last, I'm having an error while executing a job on jenkins.
I didn't see it in details.

Following the log:

Started by user anonymous
Building in workspace /home/janario/Projects/open/http-request-plugin/work/workspace/GET
HttpMode: GET
URL: http://requestb.in/1mndyoq1
Sending request to url: http://requestb.in/1mndyoq1
FATAL: loader constraint violation: loader (instance of hudson/ClassicPluginStrategy$AntClassLoader2) previously initiated loading for a different type with name "org/apache/http/params/HttpParams"
java.lang.LinkageError: loader constraint violation: loader (instance of hudson/ClassicPluginStrategy$AntClassLoader2) previously initiated loading for a different type with name "org/apache/http/params/HttpParams"
at jenkins.plugins.http_request.util.HttpClientUtil.execute(HttpClientUtil.java:106)
at jenkins.plugins.http_request.HttpRequest.performHttpRequest(HttpRequest.java:239)
at jenkins.plugins.http_request.HttpRequest.perform(HttpRequest.java:203)
at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:776)
at hudson.model.Build$BuildExecution.build(Build.java:203)
at hudson.model.Build$BuildExecution.doRun(Build.java:160)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:536)
at hudson.model.Run.execute(Run.java:1738)
at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
at hudson.model.ResourceController.execute(ResourceController.java:98)
at hudson.model.Executor.run(Executor.java:381)
Finished: FAILURE

Once again thank you all :)

martinda · 2016-04-25T03:27:55Z

@janario Glad to see you! Thanks for the feedback, I can make progress again.

The minimum Jenkins version is 1.609.3. I tried many times to keep it lower than that, but when it came to testing and to the Pipeline features, that's the best I was able to do. Regarding the parent pom at 2.3, I do not know, but it does not appear to be related to the Jenkins version itself.

Regarding your last item with the loader constraint violation, what Jenkins version are you using?

I am pushing some changes now, and I'll try to add the readResolve() (with a test) as soon as I can.

martinda · 2016-04-25T04:32:47Z

Please help me with this java.net.SocketTimeoutException error. I don't get it when I run locally. All tests pass for me.

martinda · 2016-04-28T02:28:27Z

@janario I have added the code you recommended. I have written a few test to see if I could verify the xStreamCompatibility() and the readResolve() code, but I just can't reproduce the problems in the first place, and my tests seem to do nothing useful (when I comment out the backward compatibility code, the backward compatibility tests still pass).

If you could have a look at the tests in abea7b6 and in 1611420, and give me some advice, I would really like my tests to be meaningful.

janario · 2016-04-29T02:53:42Z

@martinda
As I've seen the compatibility tests and they look ok :)

HttpRequestBackwardCompatibilityTest.defaultGlobalConfig

It passes without the compatibility methods because the xml has the same value of a new instance of HttpRequestGlobalConfig, then without getConfigFile it just create a new instance but linked to a wrong file '< jenkins_home >/jenkins.plugins.http_request.HttpRequestGlobalConfig.xml' where it was expected to be '< jenkins_home >/jenkins.plugins.http_request.HttpRequest.xml'
You can add an assertion of the file name to ensure that it's been read from the right file:

assertEquals("jenkins.plugins.http_request.HttpRequest.xml", cfg.getConfigFile().getFile().getName());

HttpRequestBackwardCompatibilityTest.populatedGlobalConfig
Seems ok and fails without the compatibility methods

HttpRequestBackwardCompatibilityTest.oldConfigWithoutCustomHeadersShouldLoad
Without the compatibility methods it will have customHeaders and validResponseCodes as null
You can add an assertion about these values

AbstractProject p = (AbstractProject) j.getInstance().getItem("old");

List<Builder> builders = ((FreeStyleProject) p).getBuilders();

HttpRequest httpRequest = (HttpRequest) builders.get(0);
assertEquals("url", httpRequest.getUrl());
assertNotNull(httpRequest.getCustomHeaders());
assertNotNull(httpRequest.getValidResponseCodes());
assertEquals("100:399", httpRequest.getValidResponseCodes());

About the exception I'm having, I've run it with 'mvn hpi:run' which starts jenkins v1.609.3
Have you tried this way?

martinda · 2016-05-04T02:53:11Z

@janario Thank you, I was able to enhance the tests and add a new test.

I am getting the same exception with mvn hpi:run, but when upgrading a standalone instance, this exception does not occur. I think it is related to the plugin dependency on httpclient. Maybe I can downgrade that dependency or remove it, but that might be complicated as I use org.apache.httpcomponents:4.5.1 for its very useful LocalServerTestBase class.

martinda · 2016-05-04T03:35:04Z

The tests and the errors in this run don't seem related to this pull request... the email being triggered is sent to the owner of PR #14 ? What's going on?

martinda · 2016-05-09T16:00:42Z

Closing to see if it clears the problem with mixing PR 13 and PR 14.

martinda · 2016-05-19T23:59:42Z

It turns out mvn hpi:run throws a variation on the following exception no matter which version of httpcomponents I use (I tried 4.2.2, 4.4.1, 4.5.1):

With 4.5.1, the exception is:

Started by user anonymous
Building in workspace /home/martin/git/jenkinsci/http-request-plugin/work/jobs/p/workspace
HttpMode: GET
URL: http://httpbin.org/
Sending request to url: http://httpbin.org/
FATAL: loader constraint violation: loader (instance of hudson/ClassicPluginStrategy$AntClassLoader2) previously initiated loading for a different type with name "org/apache/http/params/HttpParams"
java.lang.LinkageError: loader constraint violation: loader (instance of hudson/ClassicPluginStrategy$AntClassLoader2) previously initiated loading for a different type with name "org/apache/http/params/HttpParams"
at jenkins.plugins.http_request.util.HttpClientUtil.execute(HttpClientUtil.java:106)
at jenkins.plugins.http_request.HttpRequest.performHttpRequest(HttpRequest.java:249)
at jenkins.plugins.http_request.HttpRequest.perform(HttpRequest.java:213)
at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:776)
at hudson.model.Build$BuildExecution.build(Build.java:203)
at hudson.model.Build$BuildExecution.doRun(Build.java:160)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:536)
at hudson.model.Run.execute(Run.java:1738)
at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
at hudson.model.ResourceController.execute(ResourceController.java:98)
at hudson.model.Executor.run(Executor.java:381)
Finished: FAILURE

I do not understand my mistake. What is special about hpi:run?

janario · 2016-05-20T19:48:27Z

I fixed it with 'pluginFirstClassLoader' configuration.
I'll release it soon

Thanks. :)
Merged

martinda mentioned this pull request Jan 16, 2016

[WIP] Do not merge: Prepare plugin for workflow integration #12

Closed

oleg-nenashev reviewed Jan 18, 2016
View reviewed changes

martinda added 4 commits January 18, 2016 11:15

Support vim files in .gitignore

d86b7ed

Deprecate properties marked to be deprecated

c38c927

Add maven-hpi-plugin and set escape-by-default in jelly

f776b1d

Set default values to shorten call in workflow

aaf70bc

martinda force-pushed the feature/workflow-integration branch from 169b1c5 to 9708ce1 Compare January 19, 2016 21:54

amuniz reviewed Jan 26, 2016
View reviewed changes

jglick reviewed Jan 26, 2016
View reviewed changes

KostyaSha reviewed Jan 27, 2016
View reviewed changes

Martin d'Anjou added 2 commits January 27, 2016 14:02

Simplify configRoundtrip usage

f21c1e2

Use older minimal jenkins version

8200941

Bump to 1.609.3

181c792

jglick mentioned this pull request Feb 26, 2016

[JENKINS-23378] Servlet 3.1 support jenkinsci/jenkins-test-harness#12

Merged

martinda added 4 commits April 24, 2016 23:05

Fix OOME with surefire argLine -Xmx320m

38d2339

Moved martinda to contributor

ec84025

Added martinda as @author

e6e0dc5

Backward compatibility of HttpRequestGlobalConfig with XStream2

abea7b6

Fix findbug null pointer dereference

f53f4fe

martinda added 2 commits April 27, 2016 18:53

Fix test time out

9d4169e

Test job config backward compatibility

1611420

Enhance backward compatibility tests

98108c9

martinda closed this May 9, 2016

martinda reopened this May 9, 2016

janario merged commit 672e036 into jenkinsci:master May 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make plugin compatible with Workflow #13

Make plugin compatible with Workflow #13

martinda commented Jan 16, 2016

martinda commented Jan 16, 2016

jenkinsadmin commented Jan 16, 2016

oleg-nenashev Jan 18, 2016

jglick Jan 26, 2016

oleg-nenashev commented Jan 18, 2016

martinda commented Jan 18, 2016

martinda commented Jan 18, 2016

oleg-nenashev commented Jan 19, 2016

martinda commented Jan 20, 2016

amuniz Jan 26, 2016

amuniz commented Jan 26, 2016

jglick Jan 26, 2016

jglick commented Jan 26, 2016

KostyaSha Jan 27, 2016

martinda Jan 27, 2016

KostyaSha Jan 27, 2016

martinda Jan 27, 2016

martinda commented Feb 22, 2016

jglick commented Feb 22, 2016

jglick commented Feb 22, 2016

priytamaj commented Mar 29, 2016

omehegan commented Apr 4, 2016

martinda commented Apr 4, 2016

omehegan commented Apr 4, 2016

janario commented Apr 20, 2016

martinda commented Apr 25, 2016

martinda commented Apr 25, 2016

martinda commented Apr 28, 2016

janario commented Apr 29, 2016

martinda commented May 4, 2016

martinda commented May 4, 2016

martinda commented May 9, 2016

martinda commented May 19, 2016

janario commented May 20, 2016

Make plugin compatible with Workflow #13

Make plugin compatible with Workflow #13

Conversation

martinda commented Jan 16, 2016

martinda commented Jan 16, 2016

jenkinsadmin commented Jan 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jan 18, 2016

martinda commented Jan 18, 2016

martinda commented Jan 18, 2016

oleg-nenashev commented Jan 19, 2016

martinda commented Jan 20, 2016

Choose a reason for hiding this comment

amuniz commented Jan 26, 2016

Choose a reason for hiding this comment

jglick commented Jan 26, 2016

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

martinda commented Feb 22, 2016

jglick commented Feb 22, 2016

jglick commented Feb 22, 2016

priytamaj commented Mar 29, 2016

omehegan commented Apr 4, 2016

martinda commented Apr 4, 2016

omehegan commented Apr 4, 2016

janario commented Apr 20, 2016

martinda commented Apr 25, 2016

martinda commented Apr 25, 2016

martinda commented Apr 28, 2016

janario commented Apr 29, 2016

martinda commented May 4, 2016

martinda commented May 4, 2016

martinda commented May 9, 2016

martinda commented May 19, 2016

janario commented May 20, 2016