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

feat(keel): add veto-related endpoints #898

Merged
merged 3 commits into from
Sep 18, 2019

Conversation

erikmunson
Copy link
Member

Wiring up the endpoints in keel related to 'veto' plugins (spinnaker/keel#348) so we can start letting folks opt out of keel management in the UI.

@@ -52,4 +52,10 @@
@GET("/application/{application}")
Map getApplicationDetails(
@Path("application") String application, @Query("includeDetails") Boolean includeDetails);

@POST("/vetos/{name}")
Map passVetoMessage(@Path("name") String name, @Body Map<String, Object> message);
Copy link
Member Author

Choose a reason for hiding this comment

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

this endpoint... does not return anything at all. Just a status code. Do we usually do something specific re: typing responses like that? I didn't see any examples and it seems like the approach historically has been "when in doubt, reach for Map.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens now when you hit gate with a veto that doesn't exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh wow that's awesome, I get back the exact same result (an empty 200) 😆

Maybe we should throw at the Keel level if you try to use a non-existent veto plugin? Seems like right now we just silently noop

Copy link
Member

Choose a reason for hiding this comment

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

If the response is just a status code, use Response<Void> as the method return type.

Copy link
Member Author

Choose a reason for hiding this comment

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

❤️ knew somebody would have the special sauce — thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@robzienert Which Response type were you referring to? I see we use retrofit.client.Response in some places, but that doesn't take a type parameter (and using it here serializes out a bunch of verbose response metadata like headers and whatnot). Poking around, the only similar type signatures I saw were the okhttp3 one (also doesn't take a parameter) and the one from jedis we use in Kork/Clouddriver/etc.

I did finally find an example of a Gate endpoint that only returns a status code, which appears to type the internal service client as a retrofit Response, and then use void as the return type in the controller (with a pit stop in a hysterix command that returns void). Is that closer to what you're looking for?

Copy link
Member Author

Choose a reason for hiding this comment

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

aside: once spinnaker/keel#446 is out hitting this with a bad veto name will send back a 404

Copy link
Member

Choose a reason for hiding this comment

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

If you don't care about the response at all, which I guess you don't, then you can use void, otherwise you can use retrofit.client.Response to get the "raw" response object. It looks like void will suit you just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

done! thanks rob.

Copy link
Member

@robzienert robzienert left a comment

Choose a reason for hiding this comment

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

Scary business seeing Erik in the Javas.

@@ -52,4 +52,10 @@
@GET("/application/{application}")
Map getApplicationDetails(
@Path("application") String application, @Query("includeDetails") Boolean includeDetails);

@POST("/vetos/{name}")
Map passVetoMessage(@Path("name") String name, @Body Map<String, Object> message);
Copy link
Member

Choose a reason for hiding this comment

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

If the response is just a status code, use Response<Void> as the method return type.


@ApiOperation(value = "Pass a message to a veto plugin", response = Map.class)
@RequestMapping(value = "/vetos/{name}", method = POST)
void passVetoMessage(
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to just return the response here too so that it gets to the UI?

Copy link
Member Author

@erikmunson erikmunson Sep 18, 2019

Choose a reason for hiding this comment

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

I'd prefer not to — if Keel throws, keelService throws a RetrofitError which ends up getting serialized out to the client anyway, so the only difference would be what the client gets in the success case. Serialized retrofit responses are effectively garbage noise as far as the client is concerned — they just have the status code and a generic message describing the status code which is no more helpful than checking the response status directly.

The one place this does prevent useful context from surfacing is one that almost all gate endpoints suffer from today — if a downstream service sends back a 500, we dump out a useless retrofit response that just says Internal Server Error rather than pulling the response body (which usually has a nice stack trace) out of the downstream exception and serializing down to the client. We can do that here, but if that's worth doing I'd suggest we make a separate PR that adds the same behavior to all the endpoints in this controller.

@emjburns
Copy link
Contributor

LGTM

@erikmunson erikmunson merged commit 13ce30d into spinnaker:master Sep 18, 2019
@erikmunson erikmunson deleted the add-keel-veto-endpoints branch September 18, 2019 17:01
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.

4 participants