fix(github): handle paginated response #1589
Conversation
|
@dbyron-sf Could I please get a build approved? Thank you. |
|
Hi @boonware, could you update your commit message to |
|
@dbyron-sf Picking this up back up after a while on the back burner. I have updated my commit message as requested. |
boonware
changed the title
...roovy/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.groovy
Show resolved
Hide resolved
| @@ -0,0 +1,148 @@ | |||
| /* | |||
There was a problem hiding this comment.
We try to avoid new groovy code. Any chance you have the stamina to rewrite this in java?
There was a problem hiding this comment.
I wrote the tests in Groovy to be consistent as the existing test files in the package were also in Groovy, and still are (in fact all of the package's code is Groovy). Would this be a blocker on this PR?
There was a problem hiding this comment.
Sorry to say, but yes. Please no more groovy code.
| ResponseEntity<List<Map<String, String>>> response = restTemplate.getForEntity(organizationsUrl, List.class); | ||
| HttpHeaders headers = response.getHeaders(); | ||
| boolean isMember = githubOrganizationMember(organization, response.getBody()) | ||
| while (!isMember && hasNextPage(headers)) { |
There was a problem hiding this comment.
Seems like there's be less duplication with do/while as opposed to while/do.
There was a problem hiding this comment.
Groovy does not have do ... while. We can construct something similar, but the syntax is ugly, IMHO, for example: https://stackoverflow.com/a/46474198
There was a problem hiding this comment.
Unless you're fired up to convert this file to java, it's fine to leave it as is.
Issue
spinnaker/spinnaker#6768
Description
Linkheader, for example:'<https://github.com/api/v3/users/1234/orgs?page=2>; rel="next", <https://github.com/api/v3/users/1234/orgs?page=3>; rel="last"'.restTemplateto allow dependency mocking. A better design would inject this dependency, but this is outside the scope of the current issue.