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.

Sign up for GitHub

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

Jump to bottom

JobsApi.build() error messages are not reported #16

Closed
martinda opened this issue May 9, 2018 · 9 comments
Closed

JobsApi.build() error messages are not reported #16

martinda opened this issue May 9, 2018 · 9 comments
Assignees
@cdancy
Labels
enhancement
Milestone
v0.0.10

Comments

@martinda
Copy link
Collaborator

martinda commented May 9, 2018

Expected Behavior

When launching a build with an invalid build name, errors reported by Jenkins should be passed along to the caller.

Current Behavior

When launching a build with an invalid build name, a null Integer is returned.

Context

When an invalid build name is passed, there is no indication as to what was wrong other than null being returned. This forces users to setup logging and re-run the program, something most end users find difficult to do and/or too costly.

As suggested in comment #14 (comment) it probably makes sense to return a RequestStatus response rather than a plain integer.

Steps to Reproduce (for bugs)

The following groovy code reproduces the problem:

@Grab(group='com.cdancy', module='jenkins-rest', version='0.0.8')
import com.cdancy.jenkins.rest.JenkinsClient
JenkinsClient client = JenkinsClient.builder().build()
println(client.api().jobsApi().build("non-existent"))

It will print: null and there is no error message.

Your Environment

Linux command line.
@martinda
Copy link
Collaborator Author

martinda commented May 9, 2018

With logging, we can see what Jenkins returns:

225  [main] DEBUG org.jclouds.http.internal.JavaUrlHttpCommandExecutorService  - Sending request -628267950: POST http://localhost:8080/job/non-existent/build HTTP/1.1
225  [main] DEBUG jclouds.headers  - >> POST http://localhost:8080/job/non-existent/build HTTP/1.1
225  [main] DEBUG jclouds.headers  - >> Accept: application/unknown
226  [main] DEBUG jclouds.headers  - >> Authorization: Basic bWFydGluOm1hcnRpbg==
226  [main] DEBUG jclouds.headers  - >> Jenkins-Crumb: c5a1529a2d7830514365913226f3f32c
300  [main] DEBUG org.jclouds.http.internal.JavaUrlHttpCommandExecutorService  - Receiving response -628267950: HTTP/1.1 404 Not Found
300  [main] DEBUG jclouds.headers  - << HTTP/1.1 404 Not Found
301  [main] DEBUG jclouds.headers  - << Server: Jetty(9.2.z-SNAPSHOT)
301  [main] DEBUG jclouds.headers  - << X-Content-Type-Options: nosniff
301  [main] DEBUG jclouds.headers  - << Date: Wed, 09 May 2018 12:43:21 GMT
301  [main] DEBUG jclouds.headers  - << Cache-Control: must-revalidate,no-cache,no-store
301  [main] DEBUG jclouds.headers  - << Content-Type: text/html; charset=ISO-8859-1
301  [main] DEBUG jclouds.headers  - << Content-Length: 306
301  [main] DEBUG jclouds.wire  - << "<html>[\n]"
301  [main] DEBUG jclouds.wire  - << "<head>[\n]"
301  [main] DEBUG jclouds.wire  - << "<meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>[\n]"
301  [main] DEBUG jclouds.wire  - << "<title>Error 404 Not Found</title>[\n]"
301  [main] DEBUG jclouds.wire  - << "</head>[\n]"
301  [main] DEBUG jclouds.wire  - << "<body><h2>HTTP ERROR 404</h2>[\n]"
301  [main] DEBUG jclouds.wire  - << "<p>Problem accessing /job/non-existent/build. Reason:[\n]"
301  [main] DEBUG jclouds.wire  - << "<pre>    Not Found</pre></p><hr><i><small>Powered by Jetty://</small></i><hr/>[\n]"
301  [main] DEBUG jclouds.wire  - << "[\n]"
301  [main] DEBUG jclouds.wire  - << "</body>[\n]"
301  [main] DEBUG jclouds.wire  - << "</html>[\n]"

It is the HTML body content that could be returned in the response.

@cdancy cdancy self-assigned this May 9, 2018
@cdancy
Copy link
Owner

cdancy commented May 9, 2018

Yeah this would be a perfect case for some some type of ResponseWrapper. I don't know if creating a more generic IntegerResponse, which is in the same vein as ResponseWrapper just housing an Integer and could be used across endpoints, or create something more specific for this endpoint that can only be used once. Any thoughts?

@martinda
Copy link
Collaborator Author

martinda commented May 9, 2018

Thinking about it and studying code in jenkins-rest/bitbucket-rest as well.

@martinda
Copy link
Collaborator Author

Can you explain the difference you are making between a ResponseWrapper and an IntegerResponse, for example you say that IntegerResponse would be more generic than ResponseWrapper, what do you mean by that (I would have thought the other way around)?

@cdancy
Copy link
Owner

cdancy commented May 10, 2018

@martinda I shouldn't have used the term ResponseWrapper as we don't actually have something by that name. I was actually referring to the very real RequestStatus which is a wrapper around a Boolean. My thinking was that all endpoints which return an Integer would instead return an IntegerResponse and all endpoints which return a String would instead return a StringResponse and so on and so forth. But I keep going back and forth on whether to do this as it could be confusing to the end-user what a StringResponse is but it would be more clear what a BuildResponse, when hitting the build endpoint, is.

And by "more generic" I only meant that RequestStatus can be used for boolean values and IntegerResponse could be used for anything returning an integer no matter the context. Not the best choice of words I admit and probably caused more confusion than necessary. My apologies.

@martinda
Copy link
Collaborator Author

It may help to look at the REST API build process in details:

  1. Call the build API, it returns a queue id (integer) or it fails with an error
  2. Poll the queue API using the queue id, this can return different states in the QueueItem instance:
    1. build cancellation before the build even starts (do not wait anymore)
    2. build still pending (continue to wait or time out)
    3. build is executing with a build number (move on to the next step)
  3. Poll the buildInfo API using the QueueItem build number, returning a BuildInfo

Exposing the low level APIs with low level responses like IntegerResponse allows users to queue multiple builds in parallel. Users can chose to wait for build completion or not, depending on their specific requirements. A high level API is possible and would encapsulate the polling process and return the final BuildInfo.

It looks to me like doing both an IntegerResponse and the high level API returning the existing BuildInfo (in different pull-requests) is a good approach. What do you think?

@martinda
Copy link
Collaborator Author

I am attempting an implementation of IntegerResponse on this branch. I am not done, three tests are failing and I am debugging.

@cdancy
Copy link
Owner

cdancy commented May 11, 2018

Very cool and looks good so far.

@martinda martinda mentioned this issue May 11, 2018
Merged
@cdancy cdancy added this to the v0.0.10 milestone May 14, 2018
@cdancy
Copy link
Owner

cdancy commented May 14, 2018

Functionality merged per #18

@cdancy cdancy closed this as completed May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cdancy cdancy

Labels
enhancement
Projects
None yet
Milestone
v0.0.10
Development

No branches or pull requests
2 participants
@martinda @cdancy