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

Validate compatible handlers have correct version #58304

Merged

Conversation

pgomulka
Copy link
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")

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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
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
@@ -51,7 +51,7 @@
Supplier<DiscoveryNodes> nodesInCluster
) {
if (Version.CURRENT.major == 8) {
return List.of(
return validatedList(7,
Copy link
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
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.

None yet

3 participants