Skip to content

Commit

Permalink
Merge pull request #60 from anotherchrisberry/exceptional-messages
Browse files Browse the repository at this point in the history 
validate param choices, include messages in responses
  • Loading branch information
@tomaslin
tomaslin committed Jan 12, 2016
2 parents b05c5c8 + 71be68d commit cc17628
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 36 deletions.
97 changes: 62 additions & 35 deletions igor-web/src/main/groovy/com/netflix/spinnaker/igor/jenkins/BuildController.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.netflix.spinnaker.igor.jenkins.client.model.JobConfig
import com.netflix.spinnaker.igor.jenkins.client.model.QueuedJob
import groovy.transform.InheritConstructors
import groovy.util.logging.Slf4j
import org.springframework.web.bind.annotation.ExceptionHandler
import retrofit.RetrofitError
import javax.servlet.http.HttpServletRequest
import org.springframework.beans.factory.annotation.Autowired
Expand All @@ -31,11 +32,10 @@ import org.springframework.web.bind.annotation.PathVariable
import org.springframework.web.bind.annotation.RequestMapping
import org.springframework.web.bind.annotation.RequestMethod
import org.springframework.web.bind.annotation.RequestParam
import org.springframework.web.bind.annotation.ResponseStatus
import org.springframework.web.bind.annotation.RestController
import org.springframework.web.servlet.HandlerMapping
import org.yaml.snakeyaml.Yaml

import javax.servlet.http.HttpServletResponse
import java.util.concurrent.ExecutorService

@Slf4j
Expand All @@ -55,8 +55,7 @@ class BuildController {
def job = (String) request.getAttribute(
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE).split('/').drop(5).join('/')
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found")
}
Map result = objectMapper.convertValue(masters.map[master].getBuild(job, buildNumber), Map)
try {
Expand All @@ -78,15 +77,13 @@ class BuildController {
@RequestMapping(value = '/builds/queue/{master}/{item}')
QueuedJob getQueueLocation(@PathVariable String master, @PathVariable int item){
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found")
}
try {
return masters.map[master].getQueuedItem(item)
} catch (RetrofitError e) {
if (e.response?.status == HttpStatus.NOT_FOUND.value()) {
log.error("Queued job '${item}' not found for master '${master}'.")
throw new QueuedJobNotFoundException()
throw new QueuedJobNotFoundException("Queued job '${item}' not found for master '${master}'.")
}
throw e
}
Expand All @@ -97,8 +94,7 @@ class BuildController {
def job = (String) request.getAttribute(
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE).split('/').drop(4).join('/')
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found")
}
masters.map[master].getBuilds(job).list
}
Expand All @@ -110,14 +106,16 @@ class BuildController {
def job = (String) request.getAttribute(
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE).split('/').drop(4).join('/')
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found")
}

def response
def jenkinsService = masters.map[master]
JobConfig jobConfig = jenkinsService.getJobConfig(job)

if (jobConfig.parameterDefinitionList?.size() > 0) {
validateJobParameters(jobConfig, requestParams)
}
if (requestParams && jobConfig.parameterDefinitionList?.size() > 0) {
response = jenkinsService.buildWithParameters(job, requestParams)
} else if (!requestParams && jobConfig.parameterDefinitionList?.size() > 0) {
Expand All @@ -126,36 +124,42 @@ class BuildController {
} else if (!requestParams && (!jobConfig.parameterDefinitionList || jobConfig.parameterDefinitionList.size() == 0)) {
response = jenkinsService.build(job)
} else { // Jenkins will reject the build, so don't even try
log.error("job : ${job}, passing params to a job which doesn't need them")
// we should throw a BuildJobError, but I get a bytecode error : java.lang.VerifyError: Bad <init> method call from inside of a branch
throw new RuntimeException()
throw new RuntimeException("job : ${job}, passing params to a job which doesn't need them")
}

if (response.status != 201) {
log.error("Received a non-201 status when submitting job '${job}' to master '${master}'")
throw new BuildJobError()
throw new BuildJobError("Received a non-201 status when submitting job '${job}' to master '${master}'")
}

log.info("Submitted build job `${job}`")
def locationHeader = response.headers.find { it.name == "Location" }
if (!locationHeader) {
log.error("Could not find Location header for job '${job}'")
throw new QueuedJobDeterminationError()
throw new QueuedJobDeterminationError("Could not find Location header for job '${job}'")
}
def queuedLocation = locationHeader.value

queuedLocation.split('/')[-1]
}

static void validateJobParameters(JobConfig jobConfig, Map<String, String> requestParams) {
jobConfig.parameterDefinitionList.each { parameterDefinition ->
String matchingParam = requestParams[parameterDefinition.name]
if (matchingParam != null && parameterDefinition.type == 'ChoiceParameterDefinition' && !parameterDefinition.choices.contains(matchingParam)) {
throw new InvalidJobParameterException("`${matchingParam}` is not a valid choice " +
"for `${parameterDefinition.name}`. Valid choices are: ${parameterDefinition.choices.join(', ')}")
}
}
}

@RequestMapping(value = '/builds/properties/{buildNumber}/{fileName}/{master}/**')
Map<String, Object> getProperties(
@PathVariable String master,
@PathVariable Integer buildNumber, @PathVariable String fileName, HttpServletRequest request) {
def job = (String) request.getAttribute(
HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE).split('/').drop(6).join('/')
if (!masters.map.containsKey(master)) {
log.error("Could not find master '${master}' to get properties")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Could not find master '${master}' to get properties")
}
Map<String, Object> map = [:]
try {
Expand All @@ -182,30 +186,57 @@ class BuildController {
map
}

@ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "Jenkins master not found!")
@ExceptionHandler(BuildJobError.class)
void handleBuildJobError(HttpServletResponse response, BuildJobError e) throws IOException {
log.error(e.getMessage())
response.sendError(HttpStatus.BAD_REQUEST.value(), e.getMessage())
}

@ExceptionHandler(InvalidJobParameterException.class)
void handleInvalidJobParameterException(HttpServletResponse response, InvalidJobParameterException e) throws IOException {
log.error(e.getMessage())
response.sendError(HttpStatus.BAD_REQUEST.value(), e.getMessage())
}

@ExceptionHandler(value=[MasterNotFoundException.class,QueuedJobNotFoundException])
void handleNotFoundException(HttpServletResponse response, Exception e) throws IOException {
log.error(e.getMessage())
response.sendError(HttpStatus.NOT_FOUND.value(), e.getMessage())
}

@ExceptionHandler(QueuedJobDeterminationError.class)
void handleServiceUnavailableException(HttpServletResponse response, Exception e) throws IOException {
log.error(e.getMessage())
response.sendError(HttpStatus.SERVICE_UNAVAILABLE.value(), e.getMessage())
}

@ExceptionHandler(RuntimeException.class)
void handleOtherException(HttpServletResponse response, Exception e) throws IOException {
log.error(e.getMessage())
response.sendError(HttpStatus.INTERNAL_SERVER_ERROR.value(), e.getMessage())
}

@InheritConstructors
static class MasterNotFoundException extends RuntimeException {}

@ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "Queued job not found!")
@InheritConstructors
static class QueuedJobNotFoundException extends RuntimeException {}


@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "Build job failed!")
@InheritConstructors
static class BuildJobError extends RuntimeException {}

@ResponseStatus(value = HttpStatus.SERVICE_UNAVAILABLE, reason = "Failed to determine job from queued item!")
@InheritConstructors
static class QueuedJobDeterminationError extends RuntimeException {}

@InheritConstructors
static class InvalidJobParameterException extends RuntimeException {}

// LEGACY ENDPOINTS:

@RequestMapping(value = '/jobs/{master}/{job}/{buildNumber}')
Map getJobStatusLegacy(@PathVariable String master, @PathVariable String job, @PathVariable Integer buildNumber) {
if (!masters.map.containsKey(master)) {
log.error("Could not find master '${master}' to get job status for job '${job}'")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Could not find master '${master}' to get job status for job '${job}'")
}
Map result = objectMapper.convertValue(masters.map[master].getBuild(job, buildNumber), Map)
try {
Expand All @@ -227,15 +258,13 @@ class BuildController {
@RequestMapping(value = '/jobs/{master}/queue/{item}')
QueuedJob getQueueLocationLegacy(@PathVariable String master, @PathVariable int item){
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found for item '${item}'")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found for item '${item}'")
}
try {
return masters.map[master].getQueuedItem(item)
} catch (RetrofitError e) {
if (e.response?.status == HttpStatus.NOT_FOUND.value()) {
log.error("Queued job '${item}' not found for master '${master}'.")
throw new QueuedJobNotFoundException()
throw new QueuedJobNotFoundException("Queued job '${item}' not found for master '${master}'.")
}
throw e
}
Expand All @@ -244,8 +273,7 @@ class BuildController {
@RequestMapping(value = '/jobs/{master}/{job}/builds')
List<Build> getBuildsLegacy(@PathVariable String master, @PathVariable String job) {
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found for job '${job}'")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found for job '${job}'")
}
masters.map[master].getBuilds(job).list
}
Expand All @@ -255,8 +283,7 @@ class BuildController {
@PathVariable String master,
@PathVariable String job, @PathVariable Integer buildNumber, @PathVariable String fileName) {
if (!masters.map.containsKey(master)) {
log.error("Master '${master}' not found for job '${job}'")
throw new MasterNotFoundException()
throw new MasterNotFoundException("Master '${master}' not found for job '${job}'")
}
Map<String, Object> map = [:]
try {
Expand Down
7 changes: 7 additions & 0 deletions igor-web/src/main/groovy/com/netflix/spinnaker/igor/model/ParameterDefinition.groovy
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package com.netflix.spinnaker.igor.jenkins.client.model

import com.fasterxml.jackson.annotation.JsonInclude
import com.fasterxml.jackson.databind.annotation.JsonSerialize
import org.simpleframework.xml.Element
import org.simpleframework.xml.ElementList
import org.simpleframework.xml.Path
import org.simpleframework.xml.Root

Expand All @@ -26,4 +29,8 @@ class ParameterDefinition {

@Element
String type

@ElementList(entry = "choice", required = false, inline = true)
@JsonSerialize(include=JsonSerialize.Inclusion.NON_NULL)
List<String> choices
}
20 changes: 19 additions & 1 deletion igor-web/src/test/groovy/com/netflix/spinnaker/igor/jenkins/BuildControllerSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.netflix.spinnaker.igor.jenkins.client.model.QueuedJob
import com.netflix.spinnaker.igor.jenkins.service.JenkinsService
import com.squareup.okhttp.mockwebserver.MockResponse
import com.squareup.okhttp.mockwebserver.MockWebServer
import org.springframework.http.HttpStatus
import org.springframework.http.MediaType
import org.springframework.mock.web.MockHttpServletResponse
import org.springframework.test.web.servlet.MockMvc
Expand Down Expand Up @@ -178,7 +179,24 @@ class BuildControllerSpec extends Specification {
.contentType(MediaType.APPLICATION_JSON).param("foo", "bar")).andReturn().response

then:
thrown(NestedServletException)
response.status == HttpStatus.INTERNAL_SERVER_ERROR.value()
}

void 'trigger a build with an invalid choice'() {
given:
JobConfig config = new JobConfig()
config.parameterDefinitionList = [
new ParameterDefinition(type: "ChoiceParameterDefinition", name: "foo", choices: ["bar", "baz"])
]
1 * client.getJobConfig(JOB_NAME) >> config

when:
MockHttpServletResponse response = mockMvc.perform(put("/masters/${MASTER}/jobs/${JOB_NAME}")
.contentType(MediaType.APPLICATION_JSON).param("foo", "bat")).andReturn().response

then:
response.status == HttpStatus.BAD_REQUEST.value()
response.errorMessage == "`bat` is not a valid choice for `foo`. Valid choices are: bar, baz"
}

// LEGACY ENDPOINT TESTS
Expand Down
49 changes: 49 additions & 0 deletions igor-web/src/test/groovy/com/netflix/spinnaker/igor/jenkins/InfoControllerSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,21 @@ class InfoControllerSpec extends Specification {
response.contentAsString == '{"description":null,"displayName":"My-Build","name":"My-Build","buildable":true,"color":"red","url":"http://jenkins.builds.net/job/My-Build/","parameterDefinitionList":[{"defaultName":"pullRequestSourceBranch","defaultValue":"master","name":"pullRequestSourceBranch","description":null,"type":"StringParameterDefinition"},{"defaultName":"generation","defaultValue":"4","name":"generation","description":null,"type":"StringParameterDefinition"}],"upstreamProjectList":[{"name":"Upstream-Build","url":"http://jenkins.builds.net/job/Upstream-Build/","color":"blue"}],"downstreamProjectList":[{"name":"First-Downstream-Build","url":"http://jenkins.builds.net/job/First-Downstream-Build/","color":"blue"},{"name":"Second-Downstream-Build","url":"http://jenkins.builds.net/job/Second-Downstream-Build/","color":"blue"},{"name":"Third-Downstream-Build","url":"http://jenkins.builds.net/job/Third-Downstream-Build/","color":"red"}],"concurrentBuild":false}'
}

void 'is able to get a job config where a parameter includes choices'() {
given:
setResponse(getJobConfigWithChoices())

when:
MockHttpServletResponse response = mockMvc.perform(get('/jobs/master1/MY-JOB')
.accept(MediaType.APPLICATION_JSON)).andReturn().response

then:
1 * masters.map >> ['master2': [], 'build.masters.blah': [], 'master1': service]
response.contentAsString == '{"description":null,"displayName":"My-Build","name":"My-Build","buildable":true,"color":"red","url":"http://jenkins.builds.net/job/My-Build/","parameterDefinitionList":[' +
'{"defaultName":"someParam","defaultValue":"first","name":"someParam","description":null,"type":"ChoiceParameterDefinition","choices":["first","second"]}' +
'],"upstreamProjectList":[{"name":"Upstream-Build","url":"http://jenkins.builds.net/job/Upstream-Build/","color":"blue"}],"downstreamProjectList":[{"name":"First-Downstream-Build","url":"http://jenkins.builds.net/job/First-Downstream-Build/","color":"blue"},{"name":"Second-Downstream-Build","url":"http://jenkins.builds.net/job/Second-Downstream-Build/","color":"blue"},{"name":"Third-Downstream-Build","url":"http://jenkins.builds.net/job/Third-Downstream-Build/","color":"red"}],"concurrentBuild":false}'
}

void 'is able to get a job config with the folders plugin'() {
given:
setResponse(getJobConfig())
Expand Down Expand Up @@ -168,4 +183,38 @@ class InfoControllerSpec extends Specification {
'</freeStyleProject>'
}

private String getJobConfigWithChoices() {
return '<?xml version="1.0" encoding="UTF-8"?>' +
'<freeStyleProject>' +
'<description/>' +
'<displayName>My-Build</displayName>' +
'<name>My-Build</name>' +
'<url>http://jenkins.builds.net/job/My-Build/</url>' +
'<buildable>true</buildable>' +
'<color>red</color>' +
'<firstBuild><number>1966</number><url>http://jenkins.builds.net/job/My-Build/1966/</url></firstBuild>' +
'<healthReport><description>Build stability: 1 out of the last 5 builds failed.</description><iconUrl>health-60to79.png</iconUrl><score>80</score></healthReport>' +
'<inQueue>false</inQueue>' +
'<keepDependencies>false</keepDependencies>' +
'<lastBuild><number>2698</number><url>http://jenkins.builds.net/job/My-Build/2698/</url></lastBuild>' +
'<lastCompletedBuild><number>2698</number><url>http://jenkins.builds.net/job/My-Build/2698/</url></lastCompletedBuild>' +
'<lastFailedBuild><number>2698</number><url>http://jenkins.builds.net/job/My-Build/2698/</url></lastFailedBuild>' +
'<lastStableBuild><number>2697</number><url>http://jenkins.builds.net/job/My-Build/2697/</url></lastStableBuild>' +
'<lastSuccessfulBuild><number>2697</number><url>http://jenkins.builds.net/job/My-Build/2697/</url></lastSuccessfulBuild>' +
'<lastUnsuccessfulBuild><number>2698</number><url>http://jenkins.builds.net/job/My-Build/2698/</url></lastUnsuccessfulBuild>' +
'<nextBuildNumber>2699</nextBuildNumber>' +
'<property><parameterDefinition><name>someParam</name><type>ChoiceParameterDefinition</type>' +
'<defaultParameterValue><name>someParam</name><value>first</value></defaultParameterValue>' +
'<description/>' +
'<choice>first</choice><choice>second</choice>' +
'</parameterDefinition></property>' +
'<concurrentBuild>false</concurrentBuild>' +
'<downstreamProject><name>First-Downstream-Build</name><url>http://jenkins.builds.net/job/First-Downstream-Build/</url><color>blue</color></downstreamProject>' +
'<downstreamProject><name>Second-Downstream-Build</name><url>http://jenkins.builds.net/job/Second-Downstream-Build/</url><color>blue</color></downstreamProject>' +
'<downstreamProject><name>Third-Downstream-Build</name><url>http://jenkins.builds.net/job/Third-Downstream-Build/</url><color>red</color></downstreamProject>' +
'<scm/>' +
'<upstreamProject><name>Upstream-Build</name><url>http://jenkins.builds.net/job/Upstream-Build/</url><color>blue</color></upstreamProject>' +
'</freeStyleProject>'
}

}

0 comments on commit cc17628

Please sign in to comment.