Skip to content

Validate compatible handlers have correct version#58304

Merged
pgomulka merged 7 commits intoelastic:compat_rest_apifrom
pgomulka:compat/validate_handler_version
Jul 14, 2020
Merged

Validate compatible handlers have correct version#58304
pgomulka merged 7 commits intoelastic:compat_rest_apifrom
pgomulka:compat/validate_handler_version

Conversation

@pgomulka
Copy link
Copy Markdown
Contributor

@pgomulka pgomulka commented Jun 18, 2020

Validating if registered compatible rest handlers are overriding
compatibleWith method and specify the right compatible version
("197 błędów / +197")

Validating if registered compatible rest handlers are overriding
compatibleWith method and specify the right compatible version
@pgomulka pgomulka added the :Core/Infra/REST API REST infrastructure and utilities label Jun 18, 2020
@pgomulka pgomulka requested a review from jakelandis June 18, 2020 08:21
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 18, 2020
@pgomulka pgomulka self-assigned this Jun 18, 2020
) {
if (Version.CURRENT.major == 8) {
return List.of(
return validatedList(7,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 to adding validation... I would have a minor preference to have a static validateCompatibleHandlers(Collection handlers) that is located in a more central location and that assumes Version.CURRENT.major - 1 to help make this more generic.

Also, I think normal IllegalStateException or the like may be preferable to the assert here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree - IllegalStateException fits better here.
Also agree that this could be moved to some more generic place once we have more compat rest handlers in other modules. For the time being I would prefer to keep this in that class (as I don't know the right place since there are no more compat handlers in other modules yet)

@pgomulka pgomulka merged commit cacef33 into elastic:compat_rest_api Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants