Add create, update and delete protocol buffer methods.#71
Conversation
| * Delete a kubernetes API object using protocol buffer encoding. | ||
| * @param builder The builder for the response | ||
| * @param path The path to call in the API server | ||
| * @return The response received |
There was a problem hiding this comment.
Maybe document that delete returns only status and others will return object (if they do)?
There was a problem hiding this comment.
done. (changed the return type)
There was a problem hiding this comment.
Should the document for return value be the status response? ditto for other methods.
| @@ -81,31 +151,50 @@ public <T extends Message> T list(T.Builder listObj, String path) throws ApiExce | |||
| * @param path The URL path to call (e.g. /api/v1/namespaces/default/pods/pod-name) | |||
| * @return A Message of type T | |||
| */ | |||
There was a problem hiding this comment.
add documentation for new fields body, apiVersion, kind?
| * @param kind The kind of the object | ||
| * @return The response received. | ||
| */ | ||
| public <T extends Message> ObjectOrStatus<T> create(T obj, String path, String apiVersion, String kind) |
There was a problem hiding this comment.
Until we find a generic way to do it, I suggest we extract apiVersion and kind from T using reflection. I know it is not ideal, but I prefer it vs adding these two parameters to the surface of our proto APIs. ditto for other calls.
There was a problem hiding this comment.
Sadly, the fields aren't present in the proto. See the long discussion here:
https://groups.google.com/forum/#!topic/kubernetes-sig-api-machinery/deP_LDaAdVs
Otherwise I would have used reflection.
|
First round of review done. Thanks. |
brendandburns
force-pushed
the
proto
branch
from
43e1a55 to
9ee52b0
Compare
|
@mbohlool comments (mostly) addressed, ptal. Thanks! |
brendandburns
force-pushed
the
proto
branch
from
9ee52b0 to
ea2729b
Compare
|
@brendandburns I went through that thread about protos and replied there. I think grpc proxy would be nice addition to our generated clients and it could solve the problem of all clients with one generated proxy. |
|
Sure. |
mbohlool
left a comment
mbohlool
left a comment
There was a problem hiding this comment.
some more comments but nothing serious.
| * Delete a kubernetes API object using protocol buffer encoding. | ||
| * @param builder The builder for the response | ||
| * @param path The path to call in the API server | ||
| * @return The response received |
There was a problem hiding this comment.
Should the document for return value be the status response? ditto for other methods.
| * @param kind The kind of the object | ||
| * @return The response received. | ||
| */ | ||
| public <T extends Message> ObjectOrStatus<T> update(T obj, String path, String apiVersion, String kind) |
There was a problem hiding this comment.
is it possible for non-delete calls to return status?
There was a problem hiding this comment.
Yes, update in particular sometimes returns a 201 (I observed this behavior)
|
Tried to address the comments, please take another look. |
|
/lgtm I normally squash these type of PRs (when they have merge and duplicate commits and squash all in one commit make sense) to save time instead of asking author to do it. I will do the same here. Please speak up if you don't want me to do in this repo (or ever :) ) |
4 participants
Finishes off basic protocol buffer support. It's not perfect, but it's usable.
@mbohlool @lwander