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

HLRC: Add Delete License API #32586

Merged
merged 10 commits into from
Aug 14, 2018
Merged

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Aug 2, 2018

Relates to #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

small nits. great work. ty for knocking this out as well.

}

public static DeleteLicenseResponse fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a parseAcknowledged(parser) in AcknowledgedResponse that will let you remove all of the PARSER cruft here. I suspect it is also why AcknowledgedResponse.ACKNOWLEDGED is private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great. fixed in de96724

* specific language governing permissions and limitations
* under the License.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 0ac7816

* specific language governing permissions and limitations
* under the License.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 0ac7816


public class DeleteLicenseRequest extends AcknowledgedRequest<DeleteLicenseRequest> {

public DeleteLicenseRequest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this, and the readFrom/writeTo methods? I know this was copy/paste but if we dont need em i vote for cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's still needed for the transport client

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant did we need the methods override's if all they do is call super :) Not asking if we can remove this from being a TransportRequest altogether ;)

I also think that @dakrone did some work while i was on vacation consolidating these to all be just an AcknowledgedRequest, instead of actual subclasses that generally do zero extra things. You may ping him to see what he wants to see here, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, IIRC the AcknowledgedResponse is what @dakrone was reviewing. But I think the constructor, read/write methods can all be deleted and just have a null validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in fa311cd

@dakrone dakrone changed the title HLRC: Delete License API HLRC: Add Delete License API Aug 2, 2018
@hub-cap
Copy link
Contributor

hub-cap commented Aug 3, 2018

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 client.xpack().yourCommercialClient() is removed.

@ywelsch
Copy link
Contributor Author

ywelsch commented Aug 6, 2018

@karmi the CLA checker seems to not like me anymore

@ywelsch ywelsch requested a review from hub-cap August 6, 2018 15:43
@ywelsch
Copy link
Contributor Author

ywelsch commented Aug 9, 2018

@hub-cap this is ready for another review round

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

This looks good. I only had 1 nit about some override methods and constructor that may not be needed. Ill let you decide.

@ywelsch ywelsch merged commit 00b006f into elastic:master Aug 14, 2018
ywelsch added a commit that referenced this pull request Aug 14, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
…e-types

* elastic/master: (199 commits)
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
…listeners

* elastic/master:
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
jasontedor added a commit that referenced this pull request Aug 15, 2018
* 6.x: (96 commits)
  Introduce global checkpoint listeners (#32696)
  Use JDK 10 for 6.4 BWC builds (#32866)
  Remove unused imports - follow up to removal of test in issue 32855
  Removed flaky test. Looks like randomisation makes these assertions unreliable. This test is superfluous - it was added to address #32770 but it later turned out there was an existing test that just required a fix to provide the missing test coverage.
  [test] mute IndexShardTests.testDocStats
  Test: Fix forbidden uses in test framework (#32824)
  Security: remove password hash bootstrap check (#32440)
  Move validation to server for put user requests (#32471)
  [ML] Add high level REST client docs for ML put job endpoint (#32843)
  Painless: Change fqn_only to no_import (#32817)
  [test] mute testSearchWithSignificantTermsAgg
  Backport: CompletableContext class to avoid throwable (#32829)
  [TEST] Select free port for Minio (#32837)
  SCRIPTING: Support BucketAggScript return null (#32811) (#32833)
  HLRC: Add Delete License API (#32586)
  Aggregations/HL Rest client fix: missing scores (#32774)
  HLRC: migration get assistance API (#32744)
  Fix NOOP bulk updates (#32819)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  ...
@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
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