Skip to content

Allow specifying deprecation level for REST deprecation handler#80629

Merged
dakrone merged 4 commits intoelastic:masterfrom
dakrone:allow-deprecation-level-for-rest-handlers
Nov 16, 2021
Merged

Allow specifying deprecation level for REST deprecation handler#80629
dakrone merged 4 commits intoelastic:masterfrom
dakrone:allow-deprecation-level-for-rest-handlers

Conversation

@dakrone
Copy link
Copy Markdown
Member

@dakrone dakrone commented Nov 10, 2021

This allows specifying an optional level to be used for the deprecation handler registering a route.

The default remains if the level is unspecified.

Relates to #80167

This allows specifying an optional level to be used for the deprecation handler registering a route.

Relates to elastic#80167
@dakrone dakrone added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 v7.16.1 v8.1.0 labels Nov 10, 2021
@cjcenizal
Copy link
Copy Markdown
Contributor

FWIW, Kibana originally used default level values, and we decide that was a mistake. We're making level a required configuration option for our deprecations (elastic/kibana#115344). This is because we want developers to be very deliberate and thoughtful when designing a deprecation, and making a conscious decision about level is part of that process.

Copy link
Copy Markdown
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM
I left one comment

public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
if (compatibleVersionWarning == false) {
deprecationLogger.warn(DeprecationCategory.API, deprecationKey, deprecationMessage);
if (deprecationLevel == null || deprecationLevel == Level.WARN) {
Copy link
Copy Markdown
Contributor

@pgomulka pgomulka Nov 16, 2021

Choose a reason for hiding this comment

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

i was going to comment about this having a default value. But I see that a default level is different for compatible and different for "current version" deprecations.
might be worth a comment why deprecationLevel is nullable and why there is a different behaviour?

@dakrone dakrone merged commit debb882 into elastic:master Nov 16, 2021
@dakrone dakrone deleted the allow-deprecation-level-for-rest-handlers branch November 16, 2021 20:02
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Nov 16, 2021
…tic#80629)

This allows specifying an optional level to be used for the deprecation handler registering a route.

The default remains if the level is unspecified.

Relates to elastic#80167
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

💔 Backport failed

Status Branch Result
8.0
7.16 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 80629

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Nov 16, 2021
…tic#80629)

This allows specifying an optional level to be used for the deprecation handler registering a route.

The default remains if the level is unspecified.

Relates to elastic#80167

# Conflicts:
#	server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
#	server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java
#	server/src/main/java/org/elasticsearch/rest/RestController.java
#	server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java
#	server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java
#	server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
elasticsearchmachine pushed a commit that referenced this pull request Nov 16, 2021
…) (#80776)

This allows specifying an optional level to be used for the deprecation handler registering a route.

The default remains if the level is unspecified.

Relates to #80167
elasticsearchmachine pushed a commit that referenced this pull request Nov 16, 2021
…) (#80779)

This allows specifying an optional level to be used for the deprecation handler registering a route.

The default remains if the level is unspecified.

Relates to #80167

# Conflicts:
#	server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
#	server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java
#	server/src/main/java/org/elasticsearch/rest/RestController.java
#	server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java
#	server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java
#	server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
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.

6 participants