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

Feature/get build actions #29

Merged
merged 27 commits into from
Jul 26, 2018
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a6b2659
Added actions api to BuildInfo for retrieving build parameters
richabindra Jul 19, 2018
6c87f66
Added Action class to provide parameters and causes api
richabindra Jul 19, 2018
419d91a
Class added to get "causes" values in build info
richabindra Jul 19, 2018
c000735
Class added to get "parameters" values in build info
richabindra Jul 19, 2018
77c31ad
test resource file added - build info with no parameters
richabindra Jul 19, 2018
c2a1468
Mock tests for get build parameters and causes
richabindra Jul 19, 2018
265566c
Nullable annotation added to userName field
richabindra Jul 23, 2018
ef62f58
Added live test for getting build parameters of a job
richabindra Jul 23, 2018
529386b
Added live test for getting build causes of a job
richabindra Jul 23, 2018
f02e443
Added serialized "_class" item to Cause class
richabindra Jul 24, 2018
733dde3
Added serialized "_class" item to Parameter class
richabindra Jul 24, 2018
89f5ae9
Made method parameters final
richabindra Jul 24, 2018
ba324d7
Made method params final
richabindra Jul 24, 2018
ba92bf2
Test resource for build info when empty and null params
richabindra Jul 24, 2018
ac9e189
Added test for get params when empty or null params
richabindra Jul 24, 2018
22c630b
Redo test for get params when empty or null for simpler check
richabindra Jul 24, 2018
af01590
Removed unused import
richabindra Jul 24, 2018
c75a699
Check for potential null value in parameters map and convert to an em…
richabindra Jul 25, 2018
ffd3dbd
Resource build info json returns null string
richabindra Jul 25, 2018
c82f329
Add config xml with no default parameters to test empty and null params
richabindra Jul 25, 2018
f563208
Check for null string
richabindra Jul 25, 2018
5c6adbc
Test for parameters when no build parameters present
richabindra Jul 25, 2018
3295870
Test for empty or null build parameters
richabindra Jul 25, 2018
c2552c4
Move getRunningQueueItem method in a base class
richabindra Jul 26, 2018
a66ba2e
Removed getRunningQueueItem as it is moved to the base class
richabindra Jul 26, 2018
59dd3f3
Ensure queue item is running after build is triggered
richabindra Jul 26, 2018
06f4974
getBuildCauses test made user name independent
richabindra Jul 26, 2018
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import javax.inject.Singleton;

import com.google.common.collect.Lists;
import org.jclouds.http.HttpRequest;
import org.jclouds.http.HttpRequest.Builder;
import org.jclouds.rest.Binder;
Expand All @@ -42,11 +43,15 @@ public <R extends HttpRequest> R bindToRequest(final R request, final Object pro
if (prop.getKey() != null) {
String potentialKey = prop.getKey().trim();
if (potentialKey.length() > 0) {
builder.addFormParam(potentialKey, prop.getValue().toArray(new String[prop.getValue().size()]));
if (prop.getValue() == null) {
prop.setValue(Lists.newArrayList(""));
}

builder.addFormParam(potentialKey, prop.getValue().toArray(new String[prop.getValue().size()]));
}
}
}

return (R) builder.build();
}
}
}
43 changes: 43 additions & 0 deletions src/main/java/com/cdancy/jenkins/rest/domain/job/Action.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.cdancy.jenkins.rest.domain.job;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import org.jclouds.json.SerializedNames;

import java.util.List;

@AutoValue
public abstract class Action {

public abstract List<Cause> causes();

public abstract List<Parameter> parameters();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem, I am okay with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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


Action() {
}

@SerializedNames({"causes", "parameters"})
public static Action create(final List<Cause> causes, final List<Parameter> parameters) {
return new AutoValue_Action(
causes != null ? ImmutableList.copyOf(causes) : ImmutableList.<Cause>of(),
parameters != null ? ImmutableList.copyOf(parameters) : ImmutableList.<Parameter>of());
}
}

14 changes: 9 additions & 5 deletions src/main/java/com/cdancy/jenkins/rest/domain/job/BuildInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public abstract class BuildInfo {

public abstract List<Artifact> artifacts();

public abstract List<Action> actions();

public abstract boolean building();

@Nullable
Expand Down Expand Up @@ -70,14 +72,16 @@ public abstract class BuildInfo {
BuildInfo() {
}

@SerializedNames({ "artifacts", "building", "description", "displayName", "duration", "estimatedDuration",
@SerializedNames({ "artifacts", "actions", "building", "description", "displayName", "duration", "estimatedDuration",
"fullDisplayName", "id", "keepLog", "number", "queueId", "result", "timestamp", "url", "builtOn", "culprits" })
public static BuildInfo create(List<Artifact> artifacts, boolean building, String description, String displayName,
public static BuildInfo create(List<Artifact> artifacts, List<Action> actions, boolean building, String description, String displayName,
long duration, long estimatedDuration, String fullDisplayName, String id, boolean keepLog, int number,
int queueId, String result, long timestamp, String url, String builtOn, List<Culprit> culprits) {
return new AutoValue_BuildInfo(
artifacts != null ? ImmutableList.copyOf(artifacts) : ImmutableList.<Artifact> of(), building, description,
displayName, duration, estimatedDuration, fullDisplayName, id, keepLog, number, queueId, result, timestamp,
url, builtOn, culprits != null ? ImmutableList.copyOf(culprits) : ImmutableList.<Culprit> of());
artifacts != null ? ImmutableList.copyOf(artifacts) : ImmutableList.<Artifact> of(),
actions != null ? ImmutableList.copyOf(actions) : ImmutableList.<Action> of(),
building, description, displayName, duration, estimatedDuration, fullDisplayName,
id, keepLog, number, queueId, result, timestamp, url, builtOn,
culprits != null ? ImmutableList.copyOf(culprits) : ImmutableList.<Culprit> of());
}
}
45 changes: 45 additions & 0 deletions src/main/java/com/cdancy/jenkins/rest/domain/job/Cause.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.cdancy.jenkins.rest.domain.job;

import com.google.auto.value.AutoValue;
import org.jclouds.javax.annotation.Nullable;
import org.jclouds.json.SerializedNames;

@AutoValue
public abstract class Cause {

@Nullable
public abstract String clazz();

public abstract String shortDescription();

@Nullable
public abstract String userId();

@Nullable
public abstract String userName();

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Cause() {
}

@SerializedNames({"_class", "shortDescription", "userId", "userName"})
public static Cause create(final String clazz, final String shortDescription, final String userId, final String userName) {
return new AutoValue_Cause(clazz, shortDescription, userId, userName);
}
}
42 changes: 42 additions & 0 deletions src/main/java/com/cdancy/jenkins/rest/domain/job/Parameter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.cdancy.jenkins.rest.domain.job;

import com.google.auto.value.AutoValue;
import org.jclouds.javax.annotation.Nullable;
import org.jclouds.json.SerializedNames;

@AutoValue
public abstract class Parameter {

@Nullable
public abstract String clazz();

public abstract String name();

@Nullable
public abstract String value();

Parameter() {
}

@SerializedNames({"_class", "name", "value"})
public static Parameter create(final String clazz, final String name, final String value) {
return new AutoValue_Parameter(clazz, name, value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.List;
import java.util.Map;

import com.cdancy.jenkins.rest.domain.job.Cause;
import com.cdancy.jenkins.rest.domain.job.Parameter;
import com.cdancy.jenkins.rest.domain.plugins.Plugin;
import com.cdancy.jenkins.rest.domain.plugins.Plugins;
import org.testng.annotations.Test;
Expand Down Expand Up @@ -119,6 +121,12 @@ public void testGetBuildInfo() {
}

@Test(dependsOnMethods = "testGetBuildInfo")
public void testGetBuildParametersOfLastJob() {
List<Parameter> parameters = api().buildInfo(null, "DevTest", 1).actions().get(0).parameters();
assertTrue(parameters.size() == 0);
}

@Test(dependsOnMethods = "testGetBuildParametersOfLastJob")
public void testCreateJobThatAlreadyExists() {
String config = payloadFromResource("/freestyle-project.xml");
RequestStatus success = api().create(null, "DevTest", config);
Expand Down Expand Up @@ -307,6 +315,59 @@ public void testGetBuildInfoOfJobInFolder() {
assertTrue(output.queueId() == queueIdForAnotherJob.value());
}

@Test(dependsOnMethods = "testGetProgressiveText")
public void testGetBuildParametersofJob() {
List<Parameter> parameters = api().buildInfo("test-folder/test-folder-1", "JobInFolder",1).actions().get(0).parameters();
assertNotNull(parameters);
assertTrue(parameters.get(0).name().equals("SomeKey"));
assertTrue(parameters.get(0).value().equals("SomeVeryNewValue"));
}

@Test(dependsOnMethods = "testGetProgressiveText")
public void testGetBuildCausesOfJob() {
List<Cause> causes = api().buildInfo("test-folder/test-folder-1", "JobInFolder",1).actions().get(1).causes();
assertNotNull(causes);
assertTrue(causes.get(0).shortDescription().equals("Started by user admin"));
assertTrue(causes.get(0).userId().equals("admin"));
assertTrue(causes.get(0).userName().equals("admin"));
}

public void testCreateJobForEmptyAndNullParams() {
String config = payloadFromResource("/freestyle-project-empty-and-null-params.xml");
RequestStatus success = api().create(null, "JobForEmptyAndNullParams", config);
assertTrue(success.value());
}

@Test(dependsOnMethods = "testCreateJobForEmptyAndNullParams")
public void testBuildWithParametersOfJobForEmptyAndNullParams() {
Map<String, List<String>> params = new HashMap<>();
params.put("SomeKey1", Lists.newArrayList(""));
params.put("SomeKey2", null);
IntegerResponse job1 = api.jobsApi().buildWithParameters(null, "JobForEmptyAndNullParams", params);
assertNotNull(job1);
assertTrue(job1.value() > 0);
assertTrue(job1.errors().size() == 0);
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Contributor Author

@richabindra richabindra Jul 26, 2018

Choose a reason for hiding this comment

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

@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:

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.

private QueueApi api() {
return api.queueApi();
}

I tried following approaches:

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

private QueueApi api() {
return api.queueApi();
}

  1. 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:

  1. Duplicating getRunningQueueItem or similar logic in JobsApiLiveTest.
  2. JobsApiLiveTest extends QueueApiLiveTest.

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

Copy link
Collaborator

@martinda martinda Jul 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

continue;
}
List<Parameter> parameters = api().buildInfo(null, "JobForEmptyAndNullParams", 1).actions().get(0).parameters();
assertNotNull(parameters);
assertTrue(parameters.get(0).name().equals("SomeKey1"));
assertTrue(parameters.get(0).value().isEmpty());
assertTrue(parameters.get(1).name().equals("SomeKey2"));
assertTrue(parameters.get(1).value().isEmpty());
}

@Test(dependsOnMethods = "testGetBuildParametersOfJobForEmptyAndNullParams")
public void testDeleteJobForEmptyAndNullParams() {
RequestStatus success = api().delete(null, "JobForEmptyAndNullParams");
assertTrue(success.value());
}

@Test(dependsOnMethods = "testCreateFoldersInJenkins")
public void testCreateJobWithLeadingAndTrailingForwardSlashes() {
String config = payloadFromResource("/freestyle-project-no-params.xml");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,16 @@
import static org.testng.Assert.assertNull;
import static org.testng.Assert.assertTrue;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import javax.ws.rs.core.MediaType;

import com.cdancy.jenkins.rest.domain.job.Action;
import com.cdancy.jenkins.rest.domain.job.Cause;
import com.cdancy.jenkins.rest.domain.job.Parameter;
import org.testng.annotations.Test;

import com.cdancy.jenkins.rest.JenkinsApi;
Expand Down Expand Up @@ -536,6 +540,85 @@ public void testBuildJobWithParamsNonExistentJob() throws Exception {
}
}

public void testGetParams() throws Exception {
MockWebServer server = mockWebServer();

String body = payloadFromResource("/build-info.json");
server.enqueue(new MockResponse().setBody(body).setResponseCode(200));
JenkinsApi jenkinsApi = api(server.getUrl("/"));
JobsApi api = jenkinsApi.jobsApi();
try {
List<Parameter> output = api.buildInfo(null,"fish", 10).actions().get(0).parameters();
assertNotNull(output);
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"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed to all of the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

assertSent(server, "GET", "/job/fish/10/api/json");
} finally {
jenkinsApi.close();
server.shutdown();
}
}

public void testGetParamsWhenNoBuildParams() throws Exception {
MockWebServer server = mockWebServer();

String body = payloadFromResource("/build-info-no-params.json");
server.enqueue(new MockResponse().setBody(body).setResponseCode(200));
JenkinsApi jenkinsApi = api(server.getUrl("/"));
JobsApi api = jenkinsApi.jobsApi();
try {
List<Parameter> output = api.buildInfo(null,"fish", 10).actions().get(0).parameters();
assertTrue(output.size() == 0);
assertSent(server, "GET", "/job/fish/10/api/json");
} finally {
jenkinsApi.close();
server.shutdown();
}
}

public void testGetParamsWhenEmptyorNullParams() throws Exception {
MockWebServer server = mockWebServer();

String body = payloadFromResource("/build-info-empty-and-null-params.json");
server.enqueue(new MockResponse().setBody(body).setResponseCode(200));
JenkinsApi jenkinsApi = api(server.getUrl("/"));
JobsApi api = jenkinsApi.jobsApi();
try {
List<Parameter> output = api.buildInfo(null,"fish", 10).actions().get(0).parameters();
assertNotNull(output);
assertTrue(output.get(0).name().equals("bear"));
assertTrue(output.get(0).value().equals("null"));
assertTrue(output.get(1).name().equals("fish"));
assertTrue(output.get(1).value().isEmpty());
assertSent(server, "GET", "/job/fish/10/api/json");
} finally {
jenkinsApi.close();
server.shutdown();
}
}

public void testGetCause() throws Exception {
MockWebServer server = mockWebServer();

String body = payloadFromResource("/build-info-no-params.json");
server.enqueue(new MockResponse().setBody(body).setResponseCode(200));
JenkinsApi jenkinsApi = api(server.getUrl("/"));
JobsApi api = jenkinsApi.jobsApi();
try {
List<Cause> output = api.buildInfo(null,"fish", 10).actions().get(0).causes();
assertNotNull(output);
assertTrue(output.get(0).shortDescription().equals("Started by user anonymous"));
assertNull(output.get(0).userId());
assertTrue(output.get(0).userName().equals("anonymous"));
assertSent(server, "GET", "/job/fish/10/api/json");
} finally {
jenkinsApi.close();
server.shutdown();
}
}

public void testGetLastBuildNumber() throws Exception {
MockWebServer server = mockWebServer();

Expand Down
Loading