Feature/get build actions #29

richabindra · 2018-07-23T23:19:31Z

This PR addresses Issue #28.

martinda

@richabindra Thank you for this feature, looks great and useful. I have only one request at this time.

martinda · 2018-07-24T00:35:43Z

src/main/java/com/cdancy/jenkins/rest/domain/job/Action.java

+
+    public abstract List<Cause> causes();
+
+    public abstract List<Parameter> parameters();


I wonder if there is a way to make parameters a Map<String, String> instead of a plain list, it would make retrieval more natural.

There is a way we could marshal it into something else but I'd actually rather not do so and instead hand back to the user the exact response we get from jenkins for better or worse.

No problem, I am okay with that.

Maybe I should have dismissed the review... oh well.

Yes, I was confused about this too whether to leave it as is or should we convert it to something more useful for the user. But, it makes sense to leave it as is.

We're doing this mainly to keep things sane on our end with the least amount of confusion and gotchas. There are definitely good reasons to marshal this into something else but then I don't want to get into the habit of having these one-off cases where the user expects things to be a certain way and then the implementation provides something different. I'd rather hand back to the user, in AutoValue object format, exactly what was given to us by Jenkins.

Yes, agreed. So, just clarifying, I don't need to change anything in the PR?

Nothing on this end though please do address @martinda comments below and then we can merge and kick a new patch release.

martinda

Probably need to support a nullable "_class". I also suggest adding a test for empty parameter values. See the details in the comments.

martinda · 2018-07-24T15:11:14Z

src/main/java/com/cdancy/jenkins/rest/domain/job/Cause.java

+
+    @Nullable
+    public abstract String userName();
+


The src/test/resources/build-info-no-params.json contains a _class item some times. Should it be here?

There is a precedent in Plugins.java.

martinda · 2018-07-24T15:16:33Z

src/test/java/com/cdancy/jenkins/rest/features/JobsApiMockTest.java

+            assertTrue(output.get(0).name().equals("bear"));
+            assertTrue(output.get(0).value().equals("true"));
+            assertTrue(output.get(1).name().equals("fish"));
+            assertTrue(output.get(1).value().equals("salmon"));


Although unlikely to break, there should be a test for an empty parameter value. Maybe Jenkins returns null instead of an empty space.

Agreed to all of the above.

Yes, you are right. I'll incorporate the suggested changes in the next commit.

richabindra · 2018-07-24T17:56:01Z

src/test/java/com/cdancy/jenkins/rest/features/JobsApiMockTest.java

+                    parameters = action.parameters();
+                }
+            }
+            for (Parameter parameter : parameters) {


I intentionally added the test like this to provide an alternative to get(0) etc for getting params. I can change it if suggested otherwise.

Well, thanks for the tip, but I don't think it is necessary. In fact it makes the test slightly harder to read.

Agreed. Makes it harder to read. Lets just use what comes OOTB and iterate over that.

martinda

Is it possible to add live tests for null and empty params (providing Jenkins does return null and/or empty)? Thanks in advance.

martinda · 2018-07-24T21:56:43Z

src/main/java/com/cdancy/jenkins/rest/domain/job/Action.java

+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;


Nullable is not used in this file.

richabindra · 2018-07-25T15:55:35Z

I was trying to add the live test to check for null and empty param, but when I have a params map with a key and null value and I try to build the job using this param map, it keeps throwing me an NullPointerException on line 45 below:

jenkins-rest/src/main/java/com/cdancy/jenkins/rest/binders/BindMapToForm.java

Lines 42 to 45 in 00cb735

    
           if (prop.getKey() != null) { 
        
              String potentialKey = prop.getKey().trim(); 
        
              if (potentialKey.length() > 0) { 
        
                 builder.addFormParam(potentialKey, prop.getValue().toArray(new String[prop.getValue().size()]));

Using curl, I was able to pass 'null' values to the rest API. So, is there a way to send 'null' values as form parameters to the jobs API using jenkins-rest?

Also, I tried a workaround. In the above class i.e. BindMapsToForm.java, I locally tried adding a check before line 45 to convert any null value to an empty list. And that works since I explicitly avoid the null pointer exception during prop.getValue().toArray(...) conversion. But, this modifies the existing functionality. So, I am not sure which is the best way forward.

cdancy · 2018-07-25T16:00:37Z

@richabindra converting the potential null to an empty list sounds like a good idea to me.

richabindra · 2018-07-25T16:07:13Z

@cdancy I tried posting to jenkins using CURL which allowed null values but jenkins-rest does not seem to allow null values to a parameter.
By converting null to an empty list, are we not modifying the existing functionality?

martinda · 2018-07-25T16:16:14Z

@richabindra What does the curl POST look like? How are you POSTing a null value? It is an empty string, is is the string "null"?

richabindra · 2018-07-25T16:32:35Z

I believe it is a null value. I am doing this request:

curl -u <username:password> -X POST "http://jenkins.com/job/test-job/buildWithParameters?key1=null"

I think jenkins converts it to a string, since the parameter in build info JSON for the above build looks like this:

  .
  "actions" : [
    {
      "_class" : "hudson.model.ParametersAction",
      "parameters" : [
        {
          "_class" : "hudson.model.StringParameterValue",
          "name" : "key1",
          "value" : "null"
        }
      ]
    },
.
.
.

martinda · 2018-07-25T16:39:35Z

That is not a null value, it is a string with a 4-letter value that spell the word null, but the value is not null. I am looking at ParametersDefinitionProperty.java trying to see if there is a difference between null and the empty string.

martinda · 2018-07-25T17:27:12Z

After looking at the source in Jenkins and Stapler, I have no idea. I would say convert null to the empty string is the right approach. We cannot pass "null" from the UI anyway, we can only pass an empty string, and it is exported as an empty string value when Jenkins exports parameters as environment variables.

richabindra · 2018-07-25T17:31:20Z

@martinda - that's true. It is a string with the word null. I tried even with the url encoded value for null i.e. "%00", but jenkins build info shows it as an empty string. So, I think there is no easy way to pass null through the rest api.

I think it's a good idea to convert potential null to an empty list in jenkins-api as discussed earlier. What do you think. @cdancy @martinda

richabindra · 2018-07-25T17:32:18Z

Just saw your comment @martinda . Totally agree with you. I'll go forward with converting null to an empty string. Thanks!

cdancy · 2018-07-25T17:37:50Z

Just saw your comment @martinda . Totally agree with you. I'll go forward with converting null to an empty string. Thanks!

+1

…pty array list

richabindra · 2018-07-25T20:27:59Z

src/test/java/com/cdancy/jenkins/rest/features/JobsApiLiveTest.java

+
+    @Test(dependsOnMethods = "testBuildWithParametersOfJobForEmptyAndNullParams")
+    public void testGetBuildParametersOfJobForEmptyAndNullParams() {
+        while (api().jobInfo(null, "JobForEmptyAndNullParams").lastBuild() == null) {


The reason I add this check is to make sure the build has started running. If I don't add it, I am getting a null pointer exception on the next line i.e. line 357 while trying to get the parameters.

@richabindra if you look at the QueueApiLiveTest @martinda added a few tests around queuing builds that is more intuitive at the expense of a bit more code. I would use something like as this could potentially hang forever if someone is not careful. You may have to expose the method getRunningQueueItem and potentially make it static.

@cdancy - So, the thing is I am not able to make the getRunningQueueItem as static because
it refers to non static api() method at line 235:

jenkins-rest/src/test/java/com/cdancy/jenkins/rest/features/QueueApiLiveTest.java

Lines 233 to 235 in 00cb735

private QueueItem getRunningQueueItem(int queueId) throws InterruptedException {

int max = 10;

QueueItem queueItem = api().queueItem(queueId);

If I make api() method as static, it complains because it refers to api property of jclouds.apis class at line 264, which obviously I cannot make static.

jenkins-rest/src/test/java/com/cdancy/jenkins/rest/features/QueueApiLiveTest.java

Lines 263 to 265 in 00cb735

private QueueApi api() {

return api.queueApi();

}

I tried following approaches:

Calling "new QueueApiLiveTest().getRunningQueueItem(job.value())" in the test of JobsApiLiveTest, but it throws null pointer exception saying 'api' is null on line 264.

jenkins-rest/src/test/java/com/cdancy/jenkins/rest/features/QueueApiLiveTest.java

Lines 263 to 265 in 00cb735

private QueueApi api() {

return api.queueApi();

}

I created a static instance of the QueueApiLiveTest class in the QueueApiLiveTest constructor, and created a static method in QueueApiLiveTest that calls QueueApiLiveTestInstance.getRunningQueueItem(job.value()), then tried calling this static method in JobsApiLiveTest, still the 'api' on line 264 was null.

Possible alternatives:

Duplicating getRunningQueueItem or similar logic in JobsApiLiveTest.

JobsApiLiveTest extends QueueApiLiveTest.

I am not sure what's the best way possible.

@richabindra what about moving the method to the BaseJenkinsApiLiveTest class and making it protected?

@martinda - Yes, that works. I'll push the changes.

martinda · 2018-07-26T15:28:25Z

The live test fails for me, here is the relevant stacktrace:

java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:93)
	at org.testng.Assert.failNotEquals(Assert.java:512)
	at org.testng.Assert.assertTrue(Assert.java:41)
	at org.testng.Assert.assertTrue(Assert.java:51)
	at com.cdancy.jenkins.rest.features.JobsApiLiveTest.testGetBuildCausesOfJob(JobsApiLiveTest.java:333)

martinda · 2018-07-26T15:33:24Z

Ah, the live test fails because my admin user is not "admin", it is "martin". When I create the admin user, the test passes. @cdancy is that a concern?

richabindra · 2018-07-26T15:35:28Z

@martinda - ohh.. the test should be user name independent. Should I probably just check if causes list size is greater than 0?

martinda · 2018-07-26T15:42:28Z

@richabindra we're not testing for user name here, so yes the test should be independent of it. Check everything you expect the test to verify, but only that.

richabindra · 2018-07-26T15:53:21Z

@martinda - yea, good catch. Pushed the change!

martinda · 2018-07-26T17:42:37Z

Thank you @richabindra, all the tests pass for me now. This looks good! I have no further comments.

cdancy · 2018-07-26T17:46:34Z

@richabindra merged now. Thanks for the contribution! I'll spin up a new patch release shortly.

cdancy · 2018-07-26T19:47:18Z

Version 0.0.13 with this fix has been released.

richabindra added 9 commits July 19, 2018 18:30

Added actions api to BuildInfo for retrieving build parameters

a6b2659

Added Action class to provide parameters and causes api

6c87f66

Class added to get "causes" values in build info

419d91a

Class added to get "parameters" values in build info

c000735

test resource file added - build info with no parameters

77c31ad

Mock tests for get build parameters and causes

c2a1468

Nullable annotation added to userName field

265566c

Added live test for getting build parameters of a job

ef62f58

Added live test for getting build causes of a job

529386b

martinda requested changes Jul 24, 2018

View reviewed changes

martinda approved these changes Jul 24, 2018

View reviewed changes

cdancy assigned martinda and cdancy Jul 24, 2018

cdancy added the enhancement label Jul 24, 2018

cdancy added this to the v0.0.13 milestone Jul 24, 2018

martinda requested changes Jul 24, 2018

View reviewed changes

richabindra added 6 commits July 24, 2018 12:44

Added serialized "_class" item to Cause class

f02e443

Added serialized "_class" item to Parameter class

733dde3

Made method parameters final

89f5ae9

Made method params final

ba324d7

Test resource for build info when empty and null params

ba92bf2

Added test for get params when empty or null params

ac9e189

richabindra commented Jul 24, 2018

View reviewed changes

martinda requested changes Jul 24, 2018

View reviewed changes

richabindra added 2 commits July 24, 2018 19:28

Redo test for get params when empty or null for simpler check

22c630b

Removed unused import

af01590

richabindra added 6 commits July 25, 2018 16:02

Check for potential null value in parameters map and convert to an em…

c75a699

…pty array list

Resource build info json returns null string

ffd3dbd

Add config xml with no default parameters to test empty and null params

c82f329

Check for null string

f563208

Test for parameters when no build parameters present

5c6adbc

Test for empty or null build parameters

3295870

richabindra commented Jul 25, 2018

View reviewed changes

richabindra added 3 commits July 26, 2018 09:30

Move getRunningQueueItem method in a base class

c2552c4

Removed getRunningQueueItem as it is moved to the base class

a66ba2e

Ensure queue item is running after build is triggered

59dd3f3

martinda approved these changes Jul 26, 2018

View reviewed changes

getBuildCauses test made user name independent

06f4974

cdancy merged commit b7924aa into cdancy:master Jul 26, 2018


		public abstract List<Cause> causes();

		public abstract List<Parameter> parameters();

	private QueueItem getRunningQueueItem(int queueId) throws InterruptedException {
	int max = 10;
	QueueItem queueItem = api().queueItem(queueId);

Feature/get build actions #29

Feature/get build actions #29

Conversation

richabindra commented Jul 23, 2018

martinda 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

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

martinda 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richabindra commented Jul 25, 2018 • edited Loading

cdancy commented Jul 25, 2018

richabindra commented Jul 25, 2018

martinda commented Jul 25, 2018

richabindra commented Jul 25, 2018

martinda commented Jul 25, 2018 • edited Loading

martinda commented Jul 25, 2018

richabindra commented Jul 25, 2018

richabindra commented Jul 25, 2018 • edited Loading

cdancy commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richabindra Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

martinda Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinda commented Jul 26, 2018

martinda commented Jul 26, 2018

richabindra commented Jul 26, 2018

martinda commented Jul 26, 2018

richabindra commented Jul 26, 2018

martinda commented Jul 26, 2018

cdancy commented Jul 26, 2018

cdancy commented Jul 26, 2018

richabindra commented Jul 25, 2018 •

edited

Loading

martinda commented Jul 25, 2018 •

edited

Loading

richabindra commented Jul 25, 2018 •

edited

Loading

richabindra Jul 26, 2018 •

edited

Loading

martinda Jul 26, 2018 •

edited

Loading