Rest HL client: Add get license action#32438
Conversation
Continues to use String instead of a more complex License class to hold the license text similarly to put license. Relates elastic#29827
hub-cap
left a comment
hub-cap
left a comment
There was a problem hiding this comment.
I would like @nik9000 opinion here. I feel like not using a License class client side (and opting for a String instead) ended up making this code look less nice. And it puts a 1off (convertResponseToJson) in the client as well, which feels wrong. But id be happy to be told that im mistaken (spoiler alert, i usually am mistaken)!
| return response.getStatusLine().getStatusCode() == 200; | ||
| } | ||
|
|
||
| /** Converts an eniter response into a json sting |
| * This is useful for responses that we don't parse on the client side, but instead work as string | ||
| * such as in case of the license JSON | ||
| */ | ||
| static String convertResponseToJson(Response response) throws IOException { |
There was a problem hiding this comment.
Seeing this makes me wonder why we dont just return a License client side class instead of doing magic to turn the response into a string. I will let @nik9000 comment as well tho.
There was a problem hiding this comment.
I was happy with the put license API using the license json because I figured most people don't edit their license. It is just a big blob of json they get from somewhere and stick into wherever it needs to go. I mean, I'd have been happy with using the License object from x-pack:plugin:core but it has so much stuff in it! There is like a file watcher in there. All kinds of stuff that has no business being in a client at all. I feel like it'd be nice to get fields like max_nodes and expiry back in the client but License.java will take a lot of work to move. So I kind of figured it wasn't worth doing. I kind of wonder how much license administration folks will do with the java client, but :shrug:.
There was a problem hiding this comment.
How about this? I can move convertResponseToJson into LicenseClient, rename public String license() into public String getLicenseDefinition() and we will leave it like this for now. If we realize at some point that we actually want License as License, we can always implement a client-side version of the License class and add public License getLicense() method that will return it. This way we don't really paint ourselves into a corner and make progress.
There was a problem hiding this comment.
I'm fine with that. Though I still think you might be ok with EntityUtils.toString so you don't really need the helper at all.
| this.license = license; | ||
| } | ||
|
|
||
| public String license() { |
| * This is useful for responses that we don't parse on the client side, but instead work as string | ||
| * such as in case of the license JSON | ||
| */ | ||
| static String convertResponseToJson(Response response) throws IOException { |
There was a problem hiding this comment.
I was happy with the put license API using the license json because I figured most people don't edit their license. It is just a big blob of json they get from somewhere and stick into wherever it needs to go. I mean, I'd have been happy with using the License object from x-pack:plugin:core but it has so much stuff in it! There is like a file watcher in there. All kinds of stuff that has no business being in a client at all. I feel like it'd be nice to get fields like max_nodes and expiry back in the client but License.java will take a lot of work to move. So I kind of figured it wasn't worth doing. I kind of wonder how much license administration folks will do with the java client, but :shrug:.
| parser.nextToken(); | ||
| XContentBuilder builder = XContentFactory.jsonBuilder(); | ||
| builder.copyCurrentStructure(parser); | ||
| return Strings.toString(builder); |
There was a problem hiding this comment.
Would it be ok for us to do EntityUtils.toString and leave it at that? Do we need to parse the response? If we make the request with the json content-type header we can be fairly sure it'll come back json. Right?
| } | ||
|
|
||
|
|
||
| /** Converts an entire response into a json sting |
There was a problem hiding this comment.
I think this belongs one line below.
|
I have just committed #32596 which changes the location of the xpack clients. Its very likely your tests wont compile now that the xpack portion of |
Continues to use String instead of a more complex License class to hold the license text similarly to put license. Relates #29827
4 participants
WIP: @nik9000, @hub-cap I have implemented it in the license-as-a-string
style that we discussed before, but just want to make sure that we are still
ok with it for this one, before polishing it off. Could you take a look when you
have a chance?
Continues to use String instead of a more complex License class to
hold the license text similarly to put license.
Relates #29827