-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Rest HL client: Add get license action #32438
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)!
@@ -1069,6 +1077,39 @@ static boolean convertExistsResponse(Response response) { | |||
return response.getStatusLine().getStatusCode() == 200; | |||
} | |||
|
|||
/** Converts an eniter response into a json sting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entire
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be getLicense
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
} | ||
|
||
|
||
/** Converts an entire response into a json sting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this belongs one line below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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