feat(servergroups): Add endpoint to retrieve server groups by applications #1734
Conversation
jeyrschabu
commented
Jul 11, 2017
jeyrschabu
commented
- Takes a list of applications and return an aggregated summary
|
Convenient way to get a list of server groups by applications. depends on spinnaker/gate#426 @spinnaker/netflix-reviewers PTAL |
|
A couple of things:
|
|
RE @anotherchrisberry item 1: This is severe pedantry, but... I'd tend to disagree with grouping by application. The response is supposed to be a list of server groups. I'd be surprised to get a grouped response back. Maybe worth a query param to alter the response structure? |
| List getServerGroupsByApplications(@RequestParam(value = 'applications') List<String> applications, | ||
| @RequestParam(required = false, value = 'cloudProvider') String cloudProvider) { | ||
| def result = [] | ||
| applications.forEach { application -> result +=summaryList(application, cloudProvider)} |
There was a problem hiding this comment.
Could make the entire method body applications.collectMany { summaryList(it, cloudProvider) }
|
I'd also argue against a query parameter that alters the response structure ... any sort of grouping can easily be done client-side. |
|
Suppose response altering would make something like gRPC even harder. +1 |
|
sounds like a strong consensus on not grouping the response. I will address item #2 |
jeyrschabu
force-pushed
the
bulk_get_server_groups
branch
from
eea4e98
to
d1a0e64
Compare
| @@ -179,6 +186,7 @@ class ServerGroupController { | |||
| String type | |||
| String cloudProvider | |||
| String instanceType | |||
| String application | |||
There was a problem hiding this comment.
adding the application to the result
| @@ -140,6 +139,14 @@ class ServerGroupController { | |||
| return summaryList(application, cloudProvider) | |||
| } | |||
|
|
|||
| @PostFilter("hasPermission(filterObject?.application, 'APPLICATION', 'READ')") | |||
There was a problem hiding this comment.
Added a Post filter to remove unauthorized items
…tions - Takes a list of applications and return an aggregated summary
jeyrschabu
force-pushed
the
bulk_get_server_groups
branch
from
d1a0e64
to
16a177f
Compare
…r#1734) This is a rollup of a bunch of individual changes to increase validation flexibility and simplify some of the logging/reporting.
