Add optional path to jobs api #13

Dhivyaa21 · 2018-05-04T17:38:26Z

No description provided.

Dhivyaa21 · 2018-05-04T17:48:44Z

@cdancy - I will be adding more commits for the queueApi changes

cdancy · 2018-05-04T18:35:16Z

@Dhivyaa21 sounds good. Just ping me and let me know when you're ready for code review.

Dhivyaa21 · 2018-05-04T22:59:02Z

@cdancy - I think queueApi doesn't need the optional folder path. I am done with this change. Please review and let me know.

martinda · 2018-05-05T12:17:18Z

src/main/java/com/cdancy/jenkins/rest/features/JobsApi.java

 import com.cdancy.jenkins.rest.parsers.BuildNumberToInteger;
 import com.cdancy.jenkins.rest.parsers.LocationToQueueId;
 import com.cdancy.jenkins.rest.parsers.OutputToProgressiveText;
 import com.cdancy.jenkins.rest.parsers.RequestStatusParser;
+import org.jclouds.javax.annotation.Nullable;


This line does not belong here, move so it is in alphabetical order with the other org.jclouds imports.

cdancy · 2018-05-05T15:19:28Z

src/main/java/com/cdancy/jenkins/rest/features/JobsApi.java


 @RequestFilters(JenkinsAuthenticationFilter.class)
-@Path("/")
+@Path("{" + OPTIONAL_FOLDER_PATH_PARAM + "}")


A bit confused by this as it's not something the user can control at runtime. Can you explain the reasoning here?

This is the one comment stopping this PR from moving forward. If you could explain your thinking here that would be great.

I just thought the OPTIONAL_FOLDER_PATH_PARAM would be common for all the jobAPIs.(Like a global variable?)

So this is the base path and everything builds on top of it. I believe the idea is to pass this on as a method option and not set it on the base path. Do things work for you locally when you do this?

Yes, the live tests passed for me.

Ah, so this is like setting the @Path("/queue") at the class level for the QueueApi interface:

jenkins-rest/src/main/java/com/cdancy/jenkins/rest/features/QueueApi.java

Lines 44 to 51 in 705a8e2

@Path("/queue")

public interface QueueApi {

@Named("queue:queue")

@Path("/api/json")

@SelectJson("items")

@GET

List<QueueItem> queue();

@martinda exactly. It's the starting path and we build on top of it.

Do you think there could be a problem doing this?

@Dhivyaa21 I guess I don't see the reason why we would do it in 2 places is all unless I'm missing something.

@cdancy - Okay.. I think I am getting it. I made the base path a variable here whereas it is a constant in the other class. I am just trying to understand why can't the base path be a variable.

@Dhivyaa21 because we can't dynamically affect that at runtime so to me it does not make sense to have that be a variable.

@cdancy I have modified the code and added a commit to this pull-request. Please review.

martinda · 2018-05-09T11:57:04Z

src/main/java/com/cdancy/jenkins/rest/filters/ScrubNullFolderParam.java

+    public HttpRequest filter(final HttpRequest request) throws HttpException {        
+        final String requestPath = request.getEndpoint().getRawPath().replaceAll(SCRUB_NULL_PARAM, EMPTY_STRING);
+        return request.toBuilder().fromHttpRequest(request).replacePath(requestPath).build();
+    }


@cdancy @Dhivyaa21 How should the user specify the optional path when calling the JobsApi methods?

Jenkins URL style: job/folderA/job/folderB/job/jobName

Simple path expression: folderA/folderB/jobName

This method looks like the right place to convert the simple path expression to the URL. It would make life easier for the user. What do you think? Should it be this method, or should it be in another implementation of HttpRequestFilter?

Hmmm ... I suppose both are possible right? I'm open for suggestions here and yes this would be the best place IMO.

Shouldn't be too bad to do something like: ensure each "name" is preceded by job and if not then inject into string.

cdancy · 2018-05-10T10:53:47Z

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

+    public void testDeleteFolder() {
+        RequestStatus success = api().delete(null, "test-folder");
+        assertTrue(success.value());
+    }


Is there a way to tell programmatically if the plugin is installed so that we can potentially skip these tests if it isn't?

Maybe as part of a first test in this chain of tests check if we can create a folder and if we can't then skip the rest of the tests?

There is a REST API to query the list of plugins, documented here, and grep'ing for the folder plugin looks like this in curl:

curl -X GET \ "http://localhost:8080/pluginManager/api/xml?depth=1&xpath=/*/*/shortName|/*/*/version&wrapper=plugins" | grep -o -i -P "folder.*?<\/version>"

Feels like an API to add to jenkins-rest :-)

Agreed. I actually think this is a requirement before we push forward with this PR. I'll see if I can knock something out quick unless others want to do so.

Ah.. I like this idea to check if the plugin exists before we write the test.

Anddd POSTing to http://localhost:8080/pluginManager/installNecessaryPlugins would install the missing plugins as well. The http://localhost:8080/pluginManager/api page explains all the APIs.

So we could check if it's present, and if not, then attempt to install it. Very awesome.

PullRequest for PluginManagerApi put together HERE. Have a look guys and let me know what you think. It only supports the listing of the plugins which should be enough to get us around this issue and then give us something to build upon.

@Dhivyaa21 I've merged that PR to master if you want to rebase off of that and use the new PluginManagerApi.plugins() call to test if a given plugin is available.

@cdancy - sure!

This allows end user applications to configure a logger to see http transactions.

…t we can check whether or not the request contained errors or not.

… be present.

… a given folder destination.

Dhivyaa21 · 2018-05-11T21:19:01Z

@cdancy - I made a mistake rebasing off of master. Can I submit the changes for this issue in another pull-request? Sorry about that!

cdancy · 2018-05-11T22:02:18Z

Of course! I'm actually sending in another commit to implement the installNecessaryPlugins endpoint. It works but returns an http 200 no matter if the plugin ID is valid or not. If it is it will eventually install otherwise it will eventually fail. Not great but it works when it works.

cdancy · 2018-05-11T22:18:10Z

Change is now pushed to master. It might be better to attempt to install the plugin and then spin for some amount of time (maybe 5 minutes?) on the "list plugins" endpoint in hopes that it will eventually become installed.

cdancy · 2018-05-14T18:19:37Z

Not pulling in due to PR being rebased and fleshed out elsewhere.

cdancy self-assigned this May 4, 2018

cdancy added the enhancement label May 4, 2018

cdancy added this to the v0.0.7 milestone May 4, 2018

martinda reviewed May 5, 2018

View reviewed changes

cdancy mentioned this pull request May 7, 2018

JobsApi.create gained param 'optionalFolderPath' to create the job at… #11

Closed

cdancy modified the milestones: v0.0.7, v0.0.9 May 7, 2018

cdancy reviewed May 8, 2018

View reviewed changes

martinda reviewed May 9, 2018

View reviewed changes

cdancy modified the milestones: v0.0.9, v0.0.10 May 9, 2018

cdancy reviewed May 10, 2018

View reviewed changes

martinda and others added 15 commits May 11, 2018 10:14

Support passing modules to ContextBuidler

14afbe3

This allows end user applications to configure a logger to see http transactions.

Document ability to pass modules to ContextBuilder

da828c8

Example of using slf4j to trace http transactions

aeb43ec

Support passing null list of modules

ad0ddff

Do not expose the internal list of modules

e0d2e2c

Documentation will be in wiki

09d50e0

Update RELEASE_NOTES.md

a43fbb9

Prepare for release.

858d58e

Prepare for next development iteration.

41e8be1

CrumbIssuer.crumb now returns boxed version of its former self so tha…

8a0ebf3

…t we can check whether or not the request contained errors or not.

Check the returned value, not just the key, as the latter will always…

0171266

… be present.

Remove unneeded line.

0ad7371

Update RELEASE_NOTES.md

b8ef3d7

Prepare for release.

434b115

Prepare for next development iteration.

ea97fd9

cdancy and others added 12 commits May 11, 2018 10:14

Initial impl for pluginManagerApi.

35cbe10

Various fixes and tests added.

b9e3c72

Minor cleanups.

bee73f3

Update RELEASE_NOTES.md

f1a6eef

JobsApi.create gained param 'optionalFolderPath' to create the job at…

170af9b

… a given folder destination.

added optional path parameter to jobs Api

3d907a5

Live tests for the jobsApi change

54fd353

mock tests for the jobsApi change

2be84cd

add test resource

b0c3552

fix queueApiLiveTests which uses the jobsApi

1bfa08f

Put import in alphabetical order

d76ccb4

Made the base path of the JobsApi class a constant

9923881

Dhivyaa21 force-pushed the add-optional-path-to-jobsApi branch from aee3711 to 9923881 Compare May 11, 2018 21:09

cdancy closed this May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional path to jobs api #13

Add optional path to jobs api #13

Dhivyaa21 commented May 4, 2018

Dhivyaa21 commented May 4, 2018

cdancy commented May 4, 2018

Dhivyaa21 commented May 4, 2018

martinda May 5, 2018

cdancy May 5, 2018

cdancy May 8, 2018

Dhivyaa21 May 8, 2018

cdancy May 8, 2018

Dhivyaa21 May 8, 2018

martinda May 9, 2018

cdancy May 9, 2018

Dhivyaa21 May 9, 2018

cdancy May 9, 2018

Dhivyaa21 May 10, 2018

martinda May 9, 2018

cdancy May 9, 2018

cdancy May 10, 2018 •

edited

Loading

cdancy May 10, 2018

cdancy May 10, 2018

martinda May 10, 2018

cdancy May 10, 2018

Dhivyaa21 May 10, 2018

Dhivyaa21 May 10, 2018

cdancy May 10, 2018

cdancy May 10, 2018

cdancy May 11, 2018

Dhivyaa21 May 11, 2018

Dhivyaa21 commented May 11, 2018

cdancy commented May 11, 2018

cdancy commented May 11, 2018

cdancy commented May 14, 2018

	@Path("/queue")
	public interface QueueApi {

	@Named("queue:queue")
	@Path("/api/json")
	@SelectJson("items")
	@GET
	List<QueueItem> queue();

Add optional path to jobs api #13

Add optional path to jobs api #13

Conversation

Dhivyaa21 commented May 4, 2018

Dhivyaa21 commented May 4, 2018

cdancy commented May 4, 2018

Dhivyaa21 commented May 4, 2018

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

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

cdancy May 10, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dhivyaa21 commented May 11, 2018

cdancy commented May 11, 2018

cdancy commented May 11, 2018

cdancy commented May 14, 2018

cdancy May 10, 2018 •

edited

Loading