feat(monitored deploy): add endpoint to get a list of deployment monitors #3138
Conversation
| import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; | ||
| import org.springframework.web.servlet.config.annotation.EnableWebMvc; | ||
|
|
||
| @ExtendWith(SpringExtension.class) |
There was a problem hiding this comment.
@robzienert / @robfletcher I copied this pattern, but I question if we actually should test via the WebMvc? Perhaps it makes sense to validate that params are annotated correctly, but it seems like a big overhead (vs. testing the methods directly). It also makes the tests slower since a bunch of spring needs to be stood up for this style test
What are your thoughts?
There was a problem hiding this comment.
it's a java file in src/test/groovy FWIW
There was a problem hiding this comment.
going to convert it to a simple non-mvc test
| mvc.perform(request) | ||
| .andExpect(status().is2xxSuccessful()) | ||
| .andExpect( | ||
| content().json(format("[{'id':'%s','name':'%s'}]", def1.getId(), def1.getName()))); |
There was a problem hiding this comment.
there has got to be a better way than format, feedback appreciated!
There was a problem hiding this comment.
in pure Java I don't think so. But it seems like a job for def1's toString()
| ArrayList<DeploymentMonitorDefinition> result = new ArrayList<>(); | ||
|
|
||
| if (monitoredDeployConfigurationProperties != null) { | ||
| monitoredDeployConfigurationProperties |
There was a problem hiding this comment.
could streamline this with streams:
if (monitoredDeployConfigurationProperties == null) {
return Collections.emptyList();
}
return monitoredDeployConfigurationProperties
.getDeploymentMonitors()
.stream()
.map(dm -> result.add(new DeploymentMonitorDefinition(dm)))
.collect(Collectors.toList());
There was a problem hiding this comment.
thanks, i like it (even though i am not a fan of java streams)
| import lombok.Data; | ||
|
|
||
| @Data | ||
| public class DeploymentMonitorDefinition { |
There was a problem hiding this comment.
same class name but in a different package? This is triggering my anxiety
There was a problem hiding this comment.
this is the "model" for the API... should i call it something else - i don't want return all of the internal stuffs
marchello2000
force-pushed
the
mark/add_deploy_monitor_ep
branch
from
995c821
to
ea202d9
Compare
…tors Returns the names and ids of registered deployment monitors. This will be used in `deck` to let the user pick a monitor (also need to plumb through `gate`)
marchello2000
force-pushed
the
mark/add_deploy_monitor_ep
branch
from
ea202d9
to
07cfc99
Compare
expose endpoint from spinnaker/orca#3138
expose endpoint from spinnaker/orca#3138
Returns the names and ids of registered deployment monitors.
This will be used in
deckto let the user pick a monitor (also need to plumb throughgate)