add start trial API to HLRC #33406

andyb-elastic · 2018-09-05T02:02:51Z

Introduces client-specific request and response classes that do not
depend on the server

The type parameter is named licenseType in the response class to be
more descriptive. The parts that make up the acknowledged-required
response are given slightly different names than their server-response
types to be consistent with the naming in the put license API

@hub-cap I believe this is the blessed way to do this as of now, let me know if anything's not in the right place

This takes over from #32799 which I'll close

Introduces client-specific request and response classes that do not depend on the server The `type` parameter is named `licenseType` in the response class to be more descriptive. The parts that make up the acknowledged-required response are given slightly different names than their server-response types to be consistent with the naming in the put license API

elasticmachine · 2018-09-05T02:02:53Z

Pinging @elastic/es-core-infra

hub-cap

Hey great work, One thing id like to see is that we remove the HTTP verb from the name of the classes. Since this is High Level, I would like to see us move away from adding the HTTP verb, even though some of the older methods have them still. Its worth cleaning up before 7.0 imho, but lets def not add more :)

hub-cap · 2018-09-17T18:28:48Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

@@ -1278,6 +1279,26 @@ static Request deleteLicense(DeleteLicenseRequest deleteLicenseRequest) {
        return request;
    }

+    static Request postStartTrial(PostStartTrialRequest postStartTrialRequest) {


This should be in LicenseRequestConverters now

andyb-elastic · 2018-09-17T19:14:35Z

remove the HTTP verb from the name of the classes

Totally agree on this

andyb-elastic · 2018-09-17T21:28:49Z

@hub-cap I moved the request converter to LicenseRequestConverters and changed the API name from "post start trial" to "start trial"

hub-cap

Something I noticed is that somehow we do not have a LicensingIT test class that does more against a live cluster, maybe can you add that and add some random tests with all the values in the request.

client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseRequestConverters.java

client/rest-high-level/src/main/java/org/elasticsearch/client/license/StartTrialRequest.java

hub-cap · 2018-09-19T15:42:41Z

client/rest-high-level/src/main/java/org/elasticsearch/client/license/StartTrialResponse.java

+        PARSER.declareString(optionalConstructorArg(), LICENSE_TYPE_FIELD);
+        PARSER.declareString(optionalConstructorArg(), ERROR_MESSAGE_FIELD);
+        // todo consolidate this parsing with the parsing in PutLicenseResponse
+        PARSER.declareObject(optionalConstructorArg(), (parser, aVoid) -> {


I dont know a ton about this data structure here, but is it possible to use the parser's built in .map functions to return a Map<String, Object> ?

I think so, I'm not super knowledgeable about the parser facility. I'll give it a shot

It looks like XContentFactory#map works - somehow that version seems even uglier than the old school parsing code though, let me know if that's not what you meant.

It's a little tricky because the object looks like this, where the header ("message") is parsed as part of the map but is its own particular field that we pull out

{ "acknowledge": { "message": "you need to acknowledge blah blah", "security": [ "security won't be enabled blah blah" ], "watcher": [ "another acknowledge message "], } }

HAH, ill have a look.

I think that its a bit nicer, given that you cant actually parse something incorrectly. Sure its manipulating a bunch of k/v stuff in that map, but the actual parsing logic is way trappier and more subject to fail if slightly messed up. mucking with a map of list of lists is pretty low from a trappy perspective.

client/rest-high-level/src/main/java/org/elasticsearch/client/license/StartTrialResponse.java

hub-cap · 2018-09-19T16:51:06Z

Pls also add a LicenseRequestConvertersTests to test your request converter.

andyb-elastic · 2018-09-20T01:45:49Z

@hub-cap I pushed a couple commits that I believe address all your feedback

hub-cap

Sorry for the late reply. after test failures :) Left a comment about the use of the parsers builtin map() function which was the last bit of unresolved business i think.

hub-cap · 2018-09-20T15:52:29Z

client/rest-high-level/src/main/java/org/elasticsearch/client/LicenseRequestConverters.java

-            .addPathPartAsIs("_xpack")
-            .addPathPartAsIs("license")
-            .build();
+        String endpoint = new RequestConverters.EndpointBuilder().addPathPartAsIs("_xpack", "license").build();


looks great now, ty for cleaning these up as well.

hub-cap · 2018-10-15T16:48:59Z

client/rest-high-level/src/main/java/org/elasticsearch/client/license/StartTrialResponse.java

+        PARSER.declareString(optionalConstructorArg(), LICENSE_TYPE_FIELD);
+        PARSER.declareString(optionalConstructorArg(), ERROR_MESSAGE_FIELD);
+        // todo consolidate this parsing with the parsing in PutLicenseResponse
+        PARSER.declareObject(optionalConstructorArg(), (parser, aVoid) -> {


I think that its a bit nicer, given that you cant actually parse something incorrectly. Sure its manipulating a bunch of k/v stuff in that map, but the actual parsing logic is way trappier and more subject to fail if slightly messed up. mucking with a map of list of lists is pretty low from a trappy perspective.

andyb-elastic · 2018-10-16T19:34:38Z

Jenkins test this please

Introduces client-specific request and response classes that do not depend on the server The `type` parameter is named `licenseType` in the response class to be more descriptive. The parts that make up the acknowledged-required response are given slightly different names than their server-response types to be consistent with the naming in the put license API Tests do not cover all cases because the integ test cluster starts up with a trial license - this will be addressed in a future commit

andyb-elastic · 2018-10-17T16:58:04Z

master: 18aa1c1
6.x: 53b9ff1

Introduces client-specific request and response classes that do not depend on the server The `type` parameter is named `licenseType` in the response class to be more descriptive. The parts that make up the acknowledged-required response are given slightly different names than their server-response types to be consistent with the naming in the put license API Tests do not cover all cases because the integ test cluster starts up with a trial license - this will be addressed in a future commit

andyb-elastic added v7.0.0 :Core/Java High Level REST Client :Security/License License functionality for commercial features v6.5.0 labels Sep 5, 2018

andyb-elastic requested a review from hub-cap September 5, 2018 02:02

andyb-elastic mentioned this pull request Sep 5, 2018

add start trial API to HLRC #32799

Closed

hub-cap suggested changes Sep 17, 2018

View reviewed changes

andyb-elastic added 4 commits September 17, 2018 12:41

Merge branch 'master' into feature-hlrc-license-start-trial-2

8f2fa42

move converter to LicenseRequestConverters

faba897

rename post start trial to just start trial

5df4272

doc fixes

07554dd

hub-cap suggested changes Sep 19, 2018

View reviewed changes

hub-cap mentioned this pull request Sep 19, 2018

HLRC: Add support for XPack Post Start Basic Licence API #33606

Merged

andyb-elastic added 3 commits September 19, 2018 15:46

various review fixes

74afddf

use XContentParser#map to parse acknowledgement messages,

20f8c22

add hlrc license api IT

a84d80b

vladimirdolzhenko mentioned this pull request Oct 15, 2018

HLRC XPack Protocol clean up: Licence, Misc #34469

Merged

hub-cap approved these changes Oct 15, 2018

View reviewed changes

andyb-elastic added 4 commits October 15, 2018 13:05

Merge branch 'master' into feature-hlrc-license-start-trial-2

a81882d

document snag with license tests

310fc41

Merge branch 'master' into feature-hlrc-license-start-trial-2

807a0ae

consolidate license IT

7288587

andyb-elastic merged commit 18aa1c1 into elastic:master Oct 17, 2018

elasticmachine mentioned this pull request Oct 17, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

colings86 added >enhancement and removed :Security/License License functionality for commercial features labels Oct 25, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

add start trial API to HLRC #33406

add start trial API to HLRC #33406

Uh oh!

Conversation

andyb-elastic commented Sep 5, 2018

Uh oh!

elasticmachine commented Sep 5, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyb-elastic commented Sep 17, 2018

Uh oh!

andyb-elastic commented Sep 17, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hub-cap commented Sep 19, 2018

Uh oh!

andyb-elastic commented Sep 20, 2018

Uh oh!

hub-cap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyb-elastic commented Oct 16, 2018

Uh oh!

andyb-elastic commented Oct 17, 2018

Uh oh!

Uh oh!

hub-cap left a comment •

edited

Loading