fix(managed-delivery): Fix application summary API #1091
Conversation
|
Is this change backwards compatible? Ideally deploying this to gate shouldn’t depend on a specific keel software version. |
No, it's not... 😞 |
luispollo
force-pushed
the
fix-managed-application-api
branch
from
f691d3b
to
6382a8f
Compare
Now it is. |
| @@ -86,7 +86,9 @@ Response updateConstraintStatus( | |||
|
|
|||
| @GET("/application/{application}") | |||
| Map getApplicationDetails( | |||
| @Path("application") String application, @Query("includeDetails") Boolean includeDetails); | |||
| @Path("application") String application, | |||
| @Query("includeDetails") Boolean includeDetails, | |||
There was a problem hiding this comment.
What would happen if the new keel version (I.e. the one that no longer accepts this parameter) got sent includeDetails=false on the request?
There was a problem hiding this comment.
Or actually, I guess the same question applies for a value of true - that’s what deck will be sending up until we change it.
There was a problem hiding this comment.
Keel just ignores it. In other words, it no longer has any effect -- you'll have to change the UI to pass ?entities=resources in places where you previously had just ?includeDetails=true when keel changes. Until then, this would ensure the UI continues to work as before.
| return keelService.getApplicationDetails(application, includeDetails); | ||
| @RequestParam(name = "includeDetails", required = false, defaultValue = "false") | ||
| Boolean includeDetails, | ||
| @RequestParam(name = "entities", required = false, defaultValue = "") List<String> entities) { |
There was a problem hiding this comment.
can we make the default value resources here? as of right now if we ship this and then the new keel version, I think deck is going to stop getting back resource summaries. Just checked the new keel we've got up and running internally and if i hit it without an entities list I get no summaries at all.
Fixes the
/managed/applicationAPI per changes introduced in spinnaker/keel#848