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

fix(jobcontroller): correct API calls to clouddriver and remove invalid EP #764

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

marchello2000
Copy link
Contributor

  • fix(jobcontroller): getJobDetails must use a POST not a GET to clouddriver
    Currenty, calls to /applications/{app}/jobs/{account}/{region}/{name} (in gate) will fail
    with an error from clouddriver that method is not supported because gate uses GET but
    POST is expected on the clouddriver side

  • fix(jobcontroller): remove un-used, non-existent API
    ``/applications/{appName}/jobsAPI is not used (as best as I can tell) And even if it were used, it would fail because it calls to a non-existentclouddriver` API


@Headers("Accept: application/json")
@GET("/applications/{name}/jobs/{account}/{region}/{jobName}")
@POST("/applications/{name}/jobs/{account}/{region}/{jobName}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, probably change clouddriver to be a get, but [orca already depends on this being POST](https://github.com/spinnaker/orca/blob/7c4896117fdcd6930cadaa82bfdf5dcb8383f89b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/KatoRestService.groovy#L47) and seems like not a great idea to make a breaking change like that

@@ -29,14 +29,6 @@ class JobController {
@Autowired
JobService jobService

@ApiOperation(value = "Get jobs", response = List.class)
@RequestMapping(value = "/applications/{applicationName}/jobs", method = RequestMethod.GET)
List getJobs(@PathVariable String applicationName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can't find this used anywhere... and even if it were, clouddriver doesn't have this EP... maybe i am misreading something, please correct me if I am wrong

Choose a reason for hiding this comment

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

can we also do a quick check from the logs for the usage ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, didn't see anything in logs

@aravindmd
Copy link

LGTM

…river

Currenty, calls to /applications/{app}/jobs/{account}/{region}/{name} (in `gate`) will fail
with an error from `clouddriver` that method is not supported because `gate` uses `GET` but
`POST` is expected on the `clouddriver` side
``/applications/{appName}/jobs` API is not used (as best as I can tell)
And even if it were used, it would fail because it calls to a non-existent
`clouddriver` API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants