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

Deprecate HLRC EmptyResponse used by security #37540

Merged
merged 4 commits into from
Jan 24, 2019

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Jan 16, 2019

The EmptyResponse is essentially the same as returning a boolean, which
is done in other places. This commit deprecates all the existing
EmptyResponse methods and creates new boolean methods that have method
params reordered so they can exist with the deprecated methods. A
followup PR in master will remove the existing deprecated methods, fix
the parameter ordering and deprecate the incorrectly ordered parameter
methods.

Relates #36938

The EmptyResponse is essentially the same as returning a boolean, which
is done in other places. This commit deprecates all the existing
EmptyResponse methods and creates new boolean methods that have method
params reordered so they can exist with the deprecated methods. A
followup PR in master will remove the existing deprecated methods, fix
the parameter ordering and deprecate the incorrectly ordered parameter
methods.

Relates elastic#36938
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@hub-cap
Copy link
Contributor Author

hub-cap commented Jan 16, 2019

I went ahead and added the :Security/Security label so yall would be aware, feel free to remove it.

@hub-cap
Copy link
Contributor Author

hub-cap commented Jan 16, 2019

@elasticmachine run gradle build tests 1

*
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param request the request with the user to enable
* @return the response from the enable user call
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make much sense to me since it returns a boolean.

Suggested change
* @return the response from the enable user call
* @return {@code true} if the request succeeded (the user is enabled)

*
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param request the request with the user to disable
* @return the response from the enable user call
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return the response from the enable user call
* @return {@code true} if the request succeeded (the user is disabled)

*
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param request the request with the user's new password
* @return the response from the change user password call
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return the response from the change user password call
* @return {@code true} if the request succeeded (the new password was set)

@@ -26,7 +26,9 @@

/**
* Response for a request which simply returns an empty object.
@deprecated Use a boolean to instead of this class
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@deprecated Use a boolean to instead of this class
@deprecated Use a boolean instead of this class

@tvernum
Copy link
Contributor

tvernum commented Jan 16, 2019

20:39:14 > Task :client:rest-high-level:unitTest FAILED
20:39:14 Tests with failures:
20:39:14 
20:39:14   - org.elasticsearch.client.CustomRestHighLevelClientTests.testMethodsVisibility
20:39:14   - org.elasticsearch.client.RestHighLevelClientTests.testApiNamingConventions
20:39:14 FAILURE: Build failed with an exception.
20:39:14 

😿

@hub-cap
Copy link
Contributor Author

hub-cap commented Jan 17, 2019

ty for the review. Ive fixed the tests and added the wording differences you mentioned above. <3

@hub-cap
Copy link
Contributor Author

hub-cap commented Jan 17, 2019

@elasticmachine run gradle build tests 1
@elasticmachine run gradle build tests 2

hub-cap added a commit to hub-cap/elasticsearch that referenced this pull request Jan 17, 2019
This commit deprecates the few methods that had their parameters
reordered to facilitate the move from EmptyResponse to boolean. This
commit also readds the boolean based methods with the proper
signatures.

Relates elastic#37540
Relates elastic#36938
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hub-cap hub-cap merged commit 944972a into elastic:master Jan 24, 2019
hub-cap added a commit that referenced this pull request Jan 24, 2019
The EmptyResponse is essentially the same as returning a boolean, which
is done in other places. This commit deprecates all the existing
EmptyResponse methods and creates new boolean methods that have method
params reordered so they can exist with the deprecated methods. A
followup PR in master will remove the existing deprecated methods, fix
the parameter ordering and deprecate the incorrectly ordered parameter
methods.

Relates #36938
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 24, 2019
* elastic/master:
  Optimize warning header de-duplication (elastic#37725)
  Bubble exceptions up in ClusterApplierService (elastic#37729)
  SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK (elastic#37803)
  Remove unused ThreadBarrier class (elastic#37666)
  Add built-in user and role for code plugin (elastic#37030)
  Consolidate testclusters tests into a single project (elastic#37362)
  Fix docs for MappingUpdatedAction
  SQL: Introduce SQL DATE data type (elastic#37693)
  disabling bwc test while backporting elastic#37639
  Mute ClusterDisruptionIT testAckedIndexing
  Set acking timeout to 0 on dynamic mapping update (elastic#31140)
  Remove index audit output type (elastic#37707)
  Mute FollowerFailOverIT testReadRequestsReturnsLatestMappingVersion
  [ML] Increase close job timeout and lower the max number (elastic#37770)
  Remove Custom Listeners from SnapshotsService (elastic#37629)
  Use m_m_nodes from Zen1 master for Zen2 bootstrap (elastic#37701)
  Fix index filtering in follow info api. (elastic#37752)
  Use project dependency instead of substitutions for distributions (elastic#37730)
  Update authenticate to allow unknown fields (elastic#37713)
  Deprecate HLRC EmptyResponse used by security (elastic#37540)
hub-cap added a commit to hub-cap/elasticsearch that referenced this pull request Jan 25, 2019
This commit deprecates the few methods that had their parameters
reordered to facilitate the move from EmptyResponse to boolean. This
commit also readds the boolean based methods with the proper
signatures.

Relates elastic#37540
Relates elastic#36938
hub-cap added a commit that referenced this pull request Feb 4, 2019
This commit deprecates the few methods that had their parameters
reordered to facilitate the move from EmptyResponse to boolean. This
commit also readds the boolean based methods with the proper
signatures.

Relates #37540
Relates #36938
@danielmitterdorfer danielmitterdorfer removed the :Security/Security Security issues without another label label Feb 8, 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.

5 participants